-
Notifications
You must be signed in to change notification settings - Fork 4
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
perf: added palm support and changed default settings to always use e… #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as expected!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, @Asespinel.
When we create support for a new release, we usually use a perf commit instead of a feat because we typically change many things. And for this PR, we are changing a super important variable default value; it is a breaking change. In both cases (new release support and breaking changes), we want to upgrade the version to a major release (for example, if we are in v9.X.X, upgrade to v10.X.X); I suggest we use perf in the PR name.
Reviewing other things:
- Installation. (Works as expected)
- Backends.
- Github actions.
- Test cases.
- Improvements. (Are we going to support JWT tokens? Or would you like us to do that later? I think it is important because, with that improvement, we are already preparing for the Bearer deprecation, but if we don't want to do it now, we need to add it in the issues).
- Requirements. (Are we sure we want to maintain the current constraints? We have openedx_filters==0.8.0, but the version in edx-platform for palm is 1.2.0, the same with mock; here I suggest we remove the unnecessary constraints and use it only if we need to be restrictive with some package, for example, the
Django
and thepymongo
constraints are perfect because follow the edx-platform constraints, another thing can help use constraints but not too restrictive with ==, for example, maybe we want to constraint pylint to be < to 3.0.0, but no force to be a specific version to maintain congruency with edx-platform pylint version) - Documentation. (Taking into account the first comment, the right version to support palm in the readme is >=v10.0.0, and it will be helpful if you can update the drive documentation resolving the comments)
Hi @MaferMazu Thanks for the comments they are always greatly appreciated. Improvements Requirements Documentation I also modified the PR title to better suit the objective of the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for everything, @Asespinel. It looks good to me. I suggest avoiding editing the version in the init because the bumpversion workflow changes it automatically. Following #164, it is better to avoid touching that file. @Alec4r that's the only comment I have; you can merge!
This PR mainly updates the requirements for Palm support, and it modifies the setting USE_EOX_TENANT = True as I beleive we should have this as a default if eox_tenant is being installed and ready to use.
Testing instructions