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

chore: Remove edx-token-utils dependency #36077

Merged
merged 9 commits into from
Jan 24, 2025

Conversation

rijuma
Copy link
Member

@rijuma rijuma commented Jan 7, 2025

Description

There's been a request to remove edx-token-utils dependency from the repository by Axim on issue #36009. The platform only uses one function from this repository, and since this repository is outdated one of the suggestions was to remove the dependency and fold the necessary parts into edx-platform itself.

This update moves the functionality for packing/unpacking and signing JWT tokens. We only use the unpacking on edx-platform but I thought it was a good idea to have both implementations just in case.

This would have no noticeable impact user wise.

This is a 2U contribution, but I'd like to have a review from @openedx/committers-edx-platform if possible.

Supporting information

Testing instructions

  • There are no testing instructions. The application should behave the same as before this change.

Deadline

None.

@rijuma rijuma force-pushed the rijuma/remove-edx-token-utils-dep branch 12 times, most recently from 48ab9fa to ef07615 Compare January 9, 2025 15:17
@rijuma rijuma marked this pull request as ready for review January 9, 2025 16:08
@rijuma rijuma force-pushed the rijuma/remove-edx-token-utils-dep branch 5 times, most recently from 5d183d0 to c372f0c Compare January 15, 2025 19:17
@feanil feanil self-requested a review January 15, 2025 19:47
@feanil feanil self-assigned this Jan 15, 2025
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Generally looks good just a couple of small things.

cms/envs/common.py Outdated Show resolved Hide resolved
lms/envs/test.py Show resolved Hide resolved
@rijuma rijuma force-pushed the rijuma/remove-edx-token-utils-dep branch 5 times, most recently from 05f370e to 238954f Compare January 23, 2025 16:22
@rijuma rijuma force-pushed the rijuma/remove-edx-token-utils-dep branch from 238954f to 747289b Compare January 23, 2025 18:56
@rijuma rijuma requested a review from feanil January 23, 2025 18:56
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

One small nit that I'll squash into the changes and then I think this is good to merge.

lms/envs/common.py Outdated Show resolved Hide resolved
@rijuma
Copy link
Member Author

rijuma commented Jan 23, 2025

One small nit that I'll squash into the changes and then I think this is good to merge.

Done. Thanks!

@rijuma rijuma enabled auto-merge January 24, 2025 13:11
@rijuma rijuma requested a review from feanil January 24, 2025 13:12
@rijuma rijuma merged commit 05c7d26 into master Jan 24, 2025
49 checks passed
@rijuma rijuma deleted the rijuma/remove-edx-token-utils-dep branch January 24, 2025 14:00
@feanil feanil linked an issue Jan 24, 2025 that may be closed by this pull request
1 task
@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.

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.

Drop the edx-token-utils dependency
3 participants