Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHIROS-987 - upgrade flask,werkzeug to newer versions #301

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

upadhyeammit
Copy link
Contributor

@upadhyeammit upadhyeammit commented Mar 9, 2023

Why do we need this change? 💭

Documentation update? 📝

  • Yes
  • No

Security Checklist 🔒

Upon raising this PR please go through RedHatInsights/secure-coding-checklist

💂‍♂️ Checklist 🎯

  • Bugfix
  • New Feature
  • Refactor
  • Unittests Added
  • DRY code
  • Dependency Updated
  • DB Migration Added

@upadhyeammit
Copy link
Contributor Author

This updates flask to 2.2.3, I am yet to go through change matrix of 2.2.1, 2.2.2 and 2.2.3 https://github.com/pallets/flask/releases

Copy link
Collaborator

@kgaikwad kgaikwad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@upadhyeammit, PR jobs are not happy. There is failure related to pytest.

@upadhyeammit
Copy link
Contributor Author

The sqlalchemy query api has been deprecated and we will need to migrate the queries as per new 2.0 specification : https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#migration-20-query-usage . I will checking this further.

@upadhyeammit
Copy link
Contributor Author

We are using flask-script for the cmds, this is not compatible with latest flask version, instead we will need to go with flask-cli way,

https://github.com/smurfix/flask-script
https://flask.palletsprojects.com/en/2.2.x/cli/

@kgaikwad
Copy link
Collaborator

/retest

@upadhyeammit upadhyeammit force-pushed the werkzeug branch 3 times, most recently from 2d81b7a to e84407c Compare March 20, 2023 16:21
@upadhyeammit
Copy link
Contributor Author

After rebase I can see test failures :\ I am still working on to get all queries based on 2.0 spec

@upadhyeammit upadhyeammit requested a review from kgaikwad March 21, 2023 15:25
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

Patch coverage: 89.33% and project coverage change: +0.38 🎉

Comparison is base (c9f219b) 72.52% compared to head (bcba3ab) 72.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
+ Coverage   72.52%   72.91%   +0.38%     
==========================================
  Files          28       29       +1     
  Lines        1354     1377      +23     
==========================================
+ Hits          982     1004      +22     
- Misses        372      373       +1     
Flag Coverage Δ
unittests 72.91% <89.33%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ros/api/v1/recommendations.py 86.48% <66.66%> (-1.40%) ⬇️
ros/processor/insights_engine_consumer.py 69.72% <75.00%> (-1.03%) ⬇️
ros/lib/utils.py 84.97% <77.77%> (+0.97%) ⬆️
ros/processor/inventory_events_consumer.py 46.00% <80.00%> (+0.54%) ⬆️
ros/api/common/utils.py 93.22% <100.00%> (+0.62%) ⬆️
ros/api/main.py 93.75% <100.00%> (+0.89%) ⬆️
ros/api/v1/call_to_action.py 29.16% <100.00%> (ø)
ros/api/v1/hosts.py 86.52% <100.00%> (ø)
ros/api/v1/status.py 100.00% <100.00%> (ø)
ros/extensions.py 100.00% <100.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@upadhyeammit
Copy link
Contributor Author

/retest

2 similar comments
@ShivamPandya
Copy link

/retest

@upadhyeammit
Copy link
Contributor Author

/retest

@upadhyeammit
Copy link
Contributor Author

As per the discussion with @yash2189 the EE failures should not be related to this change. He is checking further on this.

@upadhyeammit
Copy link
Contributor Author

/retest

@yash2189
Copy link
Contributor

/retest

@upadhyeammit
Copy link
Contributor Author

/retest

Copy link
Contributor

@saltgen saltgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@upadhyeammit Could you add some unittests for the code changes for example, the subquery change?
IMO it would be better to assert behavior consistency this way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@upadhyeammit Shall we add the Poetry changes here, after #331 is approved? Or vice versa?

ros/lib/app.py Show resolved Hide resolved
ros/api/v1/recommendations.py Show resolved Hide resolved
ros/lib/utils.py Show resolved Hide resolved
@upadhyeammit upadhyeammit force-pushed the werkzeug branch 2 times, most recently from 1cbfbf5 to 66e1616 Compare July 3, 2023 16:36
Copy link
Contributor

@r14chandra r14chandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now 👍🏻.
Thanks @upadhyeammit for resolving the comments!

Copy link
Collaborator

@kgaikwad kgaikwad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few suggestions added. Please feel free to address those in same or separate pull-request.

Pipfile Outdated Show resolved Hide resolved
ros/api/common/utils.py Outdated Show resolved Hide resolved
ros/api/v1/hosts.py Outdated Show resolved Hide resolved
ros/api/v1/hosts.py Outdated Show resolved Hide resolved
ros/api/v1/hosts.py Outdated Show resolved Hide resolved
ros/api/v1/hosts.py Show resolved Hide resolved
ros/api/v1/recommendations.py Outdated Show resolved Hide resolved
ros/processor/insights_engine_consumer.py Outdated Show resolved Hide resolved
@kgaikwad
Copy link
Collaborator

@upadhyeammit - jobs are not happy :)

@upadhyeammit
Copy link
Contributor Author

@upadhyeammit - jobs are not happy :)

I rebased, squashed and looks like we are green!

@upadhyeammit
Copy link
Contributor Author

Thank you @kgaikwad @r14chandra and @saltgen for reviews! I am merging this one now!

@upadhyeammit upadhyeammit merged commit edcf414 into RedHatInsights:main Jul 14, 2023
@upadhyeammit upadhyeammit mentioned this pull request Jul 19, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants