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

MNTOR-2182/deprecate feature flags #4438

Merged
merged 32 commits into from
May 1, 2024
Merged

Conversation

rhelmer
Copy link
Collaborator

@rhelmer rhelmer commented Apr 25, 2024

References:

Jira: MNTOR-2182

Description

How to test

The feature flag UI now allows removing flags, which doesn't actually delete them but sets the deleted_at column in the database. Flags can be re-added.

Checklist (Definition of Done)

  • Commits in this PR are minimal and have descriptive commit messages.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@rhelmer rhelmer requested review from Vinnl, flozia and mansaj April 25, 2024 04:32
@rhelmer rhelmer requested a review from flodolo as a code owner April 25, 2024 04:32
@rhelmer rhelmer changed the title Mntor 2182/add nimbus feature rollout MNTOR-2182/add nimbus feature rollout Apr 25, 2024
@flodolo
Copy link
Collaborator

flodolo commented Apr 25, 2024

On an internationalization note: first+middle+last is a concept bound to create issues somewhere, just asking for a name might be easier. Middle name is a foreign concept even in most Western countries.
https://softwareforgood.com/why-you-should-stop-asking-for-first-and-last-names-on-forms/

@rhelmer
Copy link
Collaborator Author

rhelmer commented Apr 25, 2024

On an internationalization note: first+middle+last is a concept bound to create issues somewhere, just asking for a name might be easier. Middle name is a foreign concept even in most Western countries. https://softwareforgood.com/why-you-should-stop-asking-for-first-and-last-names-on-forms/

Thanks, I actually only had this for demo purposes and didn't mean to have it in this PR (@flozia is working on the actual implementation of this). I would prefer we didn't have separate name fields too, it's something that the provider requires in this case unfortunately.

@Vinnl
Copy link
Collaborator

Vinnl commented Apr 25, 2024

@rhelmer How would you feel about us removing the feature flag UI and infra altogether? Instead, to test locally, you could modify the default values in nimbus.yaml, and on stage and production, change the rollout in Experimenter to 0% or 100% as desired. That would ensure we have a single source of truth, and save us a bunch of code. I've worked that out in #4443, using many of the changes from this PR.

@rhelmer rhelmer changed the title MNTOR-2182/add nimbus feature rollout MNTOR-2182/deprecate feature flags Apr 25, 2024
@rhelmer
Copy link
Collaborator Author

rhelmer commented Apr 25, 2024

@rhelmer How would you feel about us removing the feature flag UI and infra altogether? Instead, to test locally, you could modify the default values in nimbus.yaml, and on stage and production, change the rollout in Experimenter to 0% or 100% as desired. That would ensure we have a single source of truth, and save us a bunch of code. I've worked that out in #4443, using many of the changes from this PR.

OK sorry I missed this comment, yes I think we could do this. Ideally I'd like to have a local environment in the nimbus config, I worry about changing defaults in local files while testing, these are too easy to accidentally commit.

@rhelmer
Copy link
Collaborator Author

rhelmer commented Apr 25, 2024

@rhelmer How would you feel about us removing the feature flag UI and infra altogether? Instead, to test locally, you could modify the default values in nimbus.yaml, and on stage and production, change the rollout in Experimenter to 0% or 100% as desired. That would ensure we have a single source of truth, and save us a bunch of code. I've worked that out in #4443, using many of the changes from this PR.

OK sorry I missed this comment, yes I think we could do this. Ideally I'd like to have a local environment in the nimbus config, I worry about changing defaults in local files while testing, these are too easy to accidentally commit.

@Vinnl I went ahead and changed the remit of this PR, it's now about deprecating feature flags. I think we need to do it in stages though since we have features shipped right now and also in review that are behind flags, let's land the cleanup and simplification of the UI (plus the delete button), and I can mark the code as deprecated. We already have a tech debt ticket to remove feature flags, so we can finalize it and remove all flags from the DB.

After that's all done, we can remove the feature flag code. We might want to leave the feature_flags table as it has a record of what was shipped and when.

@rhelmer rhelmer self-assigned this Apr 25, 2024
@rhelmer rhelmer marked this pull request as draft April 25, 2024 17:10
@rhelmer
Copy link
Collaborator Author

rhelmer commented Apr 25, 2024

Changing the purpose of this PR slightly, going to wait for #4443 to land and will rebase on top of that. This will simplify the feature flag UI and mark feature flags as deprecated, and some small related cleanup.

@rhelmer rhelmer requested review from mansaj and Vinnl April 26, 2024 03:41
@rhelmer rhelmer marked this pull request as ready for review April 26, 2024 03:41
@Vinnl
Copy link
Collaborator

Vinnl commented Apr 30, 2024

@rhelmer I landed #4443, let me know when you've rebased :)

@rhelmer
Copy link
Collaborator Author

rhelmer commented Apr 30, 2024

@rhelmer I landed #4443, let me know when you've rebased :)

It's rebased!

Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Really nice to have the deletion now too, makes it a bit cleaner. I'd maybe check with QA that they're not using the allowlist still though, just in case we're blocking their work for existing feature-flagged features.

@rhelmer
Copy link
Collaborator Author

rhelmer commented May 1, 2024

Really nice to have the deletion now too, makes it a bit cleaner. I'd maybe check with QA that they're not using the allowlist still though, just in case we're blocking their work for existing feature-flagged features.

I don't think we need the allow list for any features that are in-flight or planned, we needed it for Plus but in general I think we can just enable everything on stage. I'll add it back if I'm wrong, I just removed from UI but left the API and DB parts alone :)

@rhelmer rhelmer merged commit e795d0c into main May 1, 2024
14 checks passed
@rhelmer rhelmer deleted the MNTOR-2182/add-nimbus-feature-rollout branch May 1, 2024 19:39
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.

4 participants