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

feat: upgrade edx-drf-extensions and newrelic #4027

Merged

Conversation

robrap
Copy link
Contributor

@robrap robrap commented Sep 7, 2023

Anyone internally merging to this repository is expected to release and monitor their changes; if you are not able to do this DO NOT MERGE, please coordinate with someone who can to ensure that the changes are released.

Required Testing

  • Before deploying this change, complete a purchase in the stage environment.
    (^ We can remove that manual check once REV-2624 is done and the corresponding e2e test runs again)

Description

  • Upgrade edx-drf-extensions to 8.12.0.
  • Upgrade newrelic by removing constraint from production.in
    • Upgrade performed using a temporary constraint and pip-compile without the --upgrade to keep from upgrading unrelated requirements.

@robrap robrap requested a review from a team as a code owner September 7, 2023 01:38
Copy link
Contributor

@christopappas christopappas left a comment

Choose a reason for hiding this comment

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

will be looking for the 2nd PR that toggles it on

@robrap
Copy link
Contributor Author

robrap commented Sep 7, 2023

Cla check hung, so closing and reopening.

@robrap robrap closed this Sep 7, 2023
@robrap robrap reopened this Sep 7, 2023
@robrap
Copy link
Contributor Author

robrap commented Sep 8, 2023

[inform] @christopappas: I've got another change (under development) that I want to bring in, so I'll probably just leave this until then. or, if we want fewer changes at a time, feel free to merge this. I am not able to merge.

@robrap robrap force-pushed the robrap/upgrade-edx-drf-extensions branch from 43b33ef to ef2d7dc Compare October 17, 2023 21:36
@@ -4,7 +4,7 @@

django-ses
gunicorn==19.7.1
newrelic<5
newrelic
Copy link
Contributor Author

@robrap robrap Oct 17, 2023

Choose a reason for hiding this comment

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

Note to reviewer: This was an outdated constraint from before constraints.txt existed, and based on constraining against major version upgrades as a rule, not due to a particular known issue.

Note that the other constraints in here are likely also an issue, but are riskier so I'll them to the owning team. I figured it would be good to have the latest monitoring capabilities though, like getting logs into New Relic.

@robrap
Copy link
Contributor Author

robrap commented Oct 17, 2023

@christopappas:

  1. I upgraded edx-drf-extensions to an even newer version. (I squashed the commit, and am reusing this PR. Sorry, and hope that isn't a big deal.
  2. In a separate commit, I am upgrading newrelic.

The two commits are simple to review separately. Please re-review when you get the chance. Again, I haven't toggled anything on in ecommerce, but I do plan to once this has been deployed.

@robrap
Copy link
Contributor Author

robrap commented Oct 18, 2023

@christopappas: It looks like there is a failing test, so I'll need to look into that first.

The method generate_unversioned_payload in
edx-drf-extensions now requires a user.id to
function, so that needed to be added to the mock
user.
Upgrade edx-drf-extensions to 8.12.0. This was done
using a temporary constraint and pip-compile without
the --upgrade to keep from upgrading unrelated
requirements.
Upgrade newrelic to 9.1.0. This was done
using a temporary constraint and pip-compile without
the --upgrade to keep from upgrading unrelated
requirements.
@robrap robrap force-pushed the robrap/upgrade-edx-drf-extensions branch from c2d123a to d8e3178 Compare October 18, 2023 20:35
@@ -137,6 +137,7 @@ def generate_new_user_token(self, username, email, is_staff):
# create a mock user, and not the actual user, because we want to confirm that
# the user is created during JWT authentication
user = Mock()
user.id = 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: I updated the edx-drf-extensions CHANGELOG in openedx/edx-drf-extensions#393 to note this test utility change.

@robrap robrap changed the title feat: upgrade edx-drf-extensions feat: upgrade edx-drf-extensions and newrelic Oct 19, 2023
Copy link
Contributor

@christopappas christopappas left a comment

Choose a reason for hiding this comment

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

if oscar 3.2 doesn't go out tomorrow, and you have the time to verify the expected behavior after merged, then you're clear for takeoff

@christopappas christopappas merged commit 5ee776c into openedx:master Oct 20, 2023
9 of 10 checks passed
@robrap robrap deleted the robrap/upgrade-edx-drf-extensions branch October 20, 2023 20:42
christopappas pushed a commit that referenced this pull request Dec 4, 2023
* fix: test update for edx-drf-extensions 8.11.0

The method generate_unversioned_payload in
edx-drf-extensions now requires a user.id to
function, so that needed to be added to the mock
user.

* feat: upgrade edx-drf-extensions

Upgrade edx-drf-extensions to 8.12.0. This was done
using a temporary constraint and pip-compile without
the --upgrade to keep from upgrading unrelated
requirements.

* feat: upgrade newrelic

Upgrade newrelic to 9.1.0. This was done
using a temporary constraint and pip-compile without
the --upgrade to keep from upgrading unrelated
requirements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants