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: Commerce Coordinator step in retirement pipeline #35203

Merged
merged 11 commits into from
Aug 15, 2024

Conversation

JadeyOlivier
Copy link
Contributor

Adding caller function for new Commerce Coordinator retirement step.

@kdmccormick kdmccormick added the business-specific Code that relates to a specific user and should be refactored and removed. label Jul 30, 2024
@JadeyOlivier JadeyOlivier force-pushed the jolivier/SONIC-561/coordinator-retirement branch 3 times, most recently from 0c21b3f to 2eab8c2 Compare August 8, 2024 10:49
@kdmccormick kdmccormick removed the business-specific Code that relates to a specific user and should be refactored and removed. label Aug 8, 2024
@justinhynes
Copy link
Contributor

justinhynes commented Aug 8, 2024

I don't feel comfortable giving it a 👍 yet with the tests failing, but I think the approach here is great. I can see us using this to remove some of the other 2U-specific retirement pieces that already exist in the codebase too.

Are there plans to update the Open edX documentation with info on this new feature? I could see it being useful for other folks in the community, or even just future us. There are so many settings and configuration pieces in the monolith that configuration options and discoverability is difficult.

EDIT: Slightly worried I might send you on a wild goose chase with regards to adding new documentation. I noticed today that it seems a lot of retirement documentation exists in the /scripts/user_retirement/docs folder, so this might be the more appropriate place to document this feature?

@JadeyOlivier JadeyOlivier force-pushed the jolivier/SONIC-561/coordinator-retirement branch 2 times, most recently from c82835c to 2a02bba Compare August 13, 2024 09:46
@JadeyOlivier JadeyOlivier force-pushed the jolivier/SONIC-561/coordinator-retirement branch 4 times, most recently from dbf8832 to 2e8f62d Compare August 14, 2024 09:17
@JadeyOlivier JadeyOlivier force-pushed the jolivier/SONIC-561/coordinator-retirement branch from 2e8f62d to c06416b Compare August 14, 2024 09:25
lms/envs/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@justinhynes justinhynes left a comment

Choose a reason for hiding this comment

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

Thanks for your patience through this saga!

@JadeyOlivier JadeyOlivier merged commit 3d7cc1c into master Aug 15, 2024
49 checks passed
@JadeyOlivier JadeyOlivier deleted the jolivier/SONIC-561/coordinator-retirement branch August 15, 2024 10:44
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@HassanJaveed84
Copy link
Contributor

@JadeyOlivier @justinhynes

I believe this PR is causing our retirement pipeline to fail on requirements installation.
If it is a matter of bumping up the python version, please let us know.
We would like to fix it as soon as possible.

08:45:30 ERROR: Ignored the following versions that require a different python version: 0.15.0 Requires-Python >=3.9; 0.16.0 Requires-Python >=3.9; 5.0 Requires-Python >=3.10; 5.0.1 Requires-Python >=3.10; 5.0.2 Requires-Python >=3.10; 5.0.3 Requires-Python >=3.10; 5.0.4 Requires-Python >=3.10; 5.0.5 Requires-Python >=3.10; 5.0.6 Requires-Python >=3.10; 5.0.7 Requires-Python >=3.10; 5.0.8 Requires-Python >=3.10; 5.0a1 Requires-Python >=3.10; 5.0b1 Requires-Python >=3.10; 5.0rc1 Requires-Python >=3.10; 5.1 Requires-Python >=3.10; 5.1a1 Requires-Python >=3.10; 5.1b1 Requires-Python >=3.10; 5.1rc1 Requires-Python >=3.10
08:45:30 ERROR: Could not find a version that satisfies the requirement django-mptt==0.16.0 (from versions: 0.1, 0.2, 0.2.1, 0.3.0, 0.3.1, 0.4.0, 0.4.1, 0.4.2, 0.5.0, 0.5.1, 0.5.2, 0.5.3, 0.5.4, 0.5.5, 0.6.0, 0.6.1, 0.7.0, 0.7.1, 0.7.2, 0.7.3, 0.7.4, 0.8.0, 0.8.1, 0.8.2, 0.8.3, 0.8.4, 0.8.5, 0.8.6, 0.8.7, 0.9.0, 0.9.1, 0.10.0, 0.11.0, 0.12.0, 0.13.0, 0.13.1, 0.13.2, 0.13.3, 0.13.4, 0.14.0)
08:45:30 ERROR: No matching distribution found for django-mptt==0.16.0
08:45:30 

@justinhynes
Copy link
Contributor

Ah, yeah. I think you're correct about needing to bump the Python version.

The generic retirement feature requires the use of Django settings, and we ended up having to pull in a bunch of edx-platform requirements... which I believe are generated for Python 3.11.

Many apologies here, we should have passed this by data platform first.

Would you like me to prepare a revert? I'm not sure if @JadeyOlivier is still around as I believe they work out of SA, but I'm happy to get one staged.

@HassanJaveed84
Copy link
Contributor

Ah, yeah. I think you're correct about needing to bump the Python version.

The generic retirement feature requires the use of Django settings, and we ended up having to pull in a bunch of edx-platform requirements... which I believe are generated for Python 3.11.

Many apologies here, we should have passed this by data platform first.

Would you like me to prepare a revert? I'm not sure if @JadeyOlivier is still around as I believe they work out of SA, but I'm happy to get one staged.

Switching to 3.11 resulted in different errors.
Can we revert this for now so we can better test it before rolling out ?

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.

6 participants