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

fix: disable all environments when reviving a feature #4999

Merged
merged 12 commits into from
Oct 13, 2023

Conversation

andreas-unleash
Copy link
Contributor

Disable all environments when reviving a feature

Closes # SR-93

Signed-off-by: andreas-unleash <andreas@getunleash.ai>
@vercel
Copy link

vercel bot commented Oct 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Oct 13, 2023 7:25am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Oct 13, 2023 7:25am

@@ -362,6 +362,11 @@ export default class FeatureToggleStore implements IFeatureToggleStore {
.where({ name })
.update({ archived_at: null })
.returning(FEATURE_COLUMNS);

await this.db(FEATURE_ENVIRONMENTS_TABLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be part of the archive method, not revive.
Also I'm not sure whether it should be done at the store level or service level.
Finally since we have write operations on the write model we need to wrap the whole service method in a transaction. If you decide to do it as part of archive and bulk archive then I'd wrap it in a tx in a controller doing the archive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem right now with doing it at archive is that already archived features will not get it - We could have a migration that disables the environments for already archived features, but I would rather not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point on the transaction wrapping

Copy link
Contributor

Choose a reason for hiding this comment

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

so maybe both? in the long term it makes more sense to me to do it as part of archive. We'll probably also clean the dependencies in the archive so having one "cleanup" location would be nice.
what's the problem with a db migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only problem I see with the migration is that it will not be reversible

Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
@andreas-unleash
Copy link
Contributor Author

Moved the functionality to the service layer and wrapped in disableAllEnvsOnRevive flag

src/test/e2e/api/admin/feature-archive.e2e.test.ts Outdated Show resolved Hide resolved
src/test/e2e/api/admin/archive.test.ts Outdated Show resolved Hide resolved
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
@andreas-unleash andreas-unleash merged commit a9a75d5 into main Oct 13, 2023
8 checks passed
@andreas-unleash andreas-unleash deleted the feat/reviving-a-feature-should-disable-all-envs branch October 13, 2023 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants