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

task: changing variants blocked by cr #2966

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Conversation

chriswk
Copy link
Contributor

@chriswk chriswk commented Jan 23, 2023

About the changes

This PR adds two new functions that is protected by CR. When used instead of the current setVariantOnEnv and setVariantsOnEnv if the flag UNLEASH_EXPERIMENTAL_CR_ON_VARIANTS is set, the call is blocked. This leaves the old functions, which is used from the CR flow in place, and adds new methods protected by CR.

Also adds e2e tests verifying that the methods will block requests if CR is enabled for project:environment pair, as well as not block if CR is not enabled. Tests already in place should confirm that the default flow, without the flag enabled just works.

This PR adds two new functions that is protected by CR. When used
instead of the current setVariantOnEnv and setVariantsOnEnv if the flag
UNLEASH_EXPERIMENTAL_CR_ON_VARIANTS is set, the call is blocked. This
leaves the old functions, which is used from the CR flow in place, and
adds new methods protected by CR.
@vercel
Copy link

vercel bot commented Jan 23, 2023

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

Name Status Preview Comments Updated
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 23, 2023 at 1:38PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
unleash-docs ⬜️ Ignored (Inspect) Jan 23, 2023 at 1:38PM (UTC)

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

LGTM

Tests are intense here (don't see another way to do them without changing the scope of this PR). Might be an indication we need to the thing and make unit testing a more first class citizen here in the near future

@chriswk
Copy link
Contributor Author

chriswk commented Jan 23, 2023

Tbh, this could be a unit test as well with mocker objects, but I'd still like to have the e2e tests for this considering this could break a major workflow if it doesn't work. I can add a unit test with mockstores as well. What do you think?

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

I rarely say this, but I'd be ok if we test this manually after merging because we're using some existing methods that we know how they work...

for (const env of environments) {
await this.stopWhenChangeRequestsEnabled(projectId, env);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking if we're missing something here... imagining we have CR enabled in some environment and we're trying to push certain variants configuration to all environments. This would require you to know which envs have change request and which don't...

Perhaps this is something for the UI to split the Push into 2 sets, one for the CR endpoint another for the variants endpoint... Not convinced about this idea either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think it would be nice to have a method that accepts all envs here, so we don't need to do n queries.

@chriswk chriswk merged commit c961374 into main Jan 24, 2023
@chriswk chriswk deleted the task/enableCrOnVariantsWithFlag branch January 24, 2023 09:43
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

3 participants