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

[BD-21] Get rid of deprecated waffle namespace classes #120

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Nov 5, 2020

The WaffleSwitchNamespace class is going to be deprecated soon.
edx-platform needs this app to stop using the namespace class in order
to upgrade to the latest edx-toggles release.

JIRA: https://openedx.atlassian.net/wiki/spaces/COMM/pages/1596358943/BD-21+Toggles+Settings+Documentation

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Note that this will break unit tests in edx-platform.

@openedx-webhooks
Copy link

Thanks for the pull request, @regisb! I've created BLENDED-653 to keep track of it in Jira. More details are on the BD-21 project page.

When this pull request is ready, tag your edX technical lead.

@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Nov 5, 2020
@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from d38dee1 to 7e009a1 Compare November 5, 2020 09:31
@regisb
Copy link
Contributor Author

regisb commented Nov 5, 2020

This is ready for review @robrap.

@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from 7e009a1 to 73d78b1 Compare November 5, 2020 12:59
CHANGELOG.rst Show resolved Hide resolved
completion/__init__.py Outdated Show resolved Hide resolved
@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from e8af9c3 to 3b0df65 Compare November 5, 2020 21:25
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Reminder to update dependencies once it has been released. I will update the PR description as well.

The WaffleSwitchNamespace class is going to be deprecated soon.
edx-platform needs this app to stop using the namespace class in order
to upgrade to the latest edx-toggles release.
@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from 3b0df65 to cdf334b Compare November 6, 2020 11:39
@robrap
Copy link
Contributor

robrap commented Nov 6, 2020

Is there any advantage to making this work with the __futures__ version so it isn't blocked on edx-toggles 2.0.0 to complete, or do you want to leave this blocked until edx-toggles 2.0.0?

@regisb
Copy link
Contributor Author

regisb commented Nov 11, 2020

Is there any advantage to making this work with the futures version so it isn't blocked on edx-toggles 2.0.0 to complete, or do you want to leave this blocked until edx-toggles 2.0.0?

Good question. It would make it easier to run CI on this PR, as we would only have to wait for the the release of edx-toggles==1.2.0. But we should not release this too early; otherwise, if people need to make changes to edx-completion, they would also need to upgrade edx-platform and the applications (if any) that make use of the deprecated waffle() function.

requirements/base.in Outdated Show resolved Hide resolved
WAFFLE_NAMESPACE = "completion"


def waffle():
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is leaving this, setting a deprecated custom attribute, and making this v3.2.6 instead of v4.0.0 with no backward compatible changes. That way, if we missed anything where this was used outside of tests, we could catch it before its removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this breaks anything then the fix should be relatively straightforward. edx-platform should not be impacted, so we should only be concerned by other applications. I think this is a reasonable risk.

@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from 7b48b64 to a694142 Compare November 13, 2020 08:26
@regisb
Copy link
Contributor Author

regisb commented Nov 17, 2020

@robrap Can we merge this now that edx-toggles has been upgraded to 1.2.0 on edx-platform?

CHANGELOG.rst Outdated Show resolved Hide resolved
@robrap
Copy link
Contributor

robrap commented Nov 17, 2020

This can merge after the minor changelog fix.

This is possible by making use of the `__future__` module.
@regisb regisb force-pushed the regisb/deprecate-waffle-namespace branch from a694142 to efcb3b8 Compare November 19, 2020 07:57
@regisb
Copy link
Contributor Author

regisb commented Nov 19, 2020

@robrap done 👍

@robrap robrap merged commit 1ecd2d6 into openedx:master Nov 19, 2020
@regisb regisb deleted the regisb/deprecate-waffle-namespace branch November 20, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants