feat: include CR envs enabled on creation in event and update validation #6931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This propagates those properties to the event etc.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
thomasheartman
commented
Apr 25, 2024
Comment on lines
+21
to
+27
| environments: joi.array().items(joi.string()), | ||
| changeRequestEnvironments: joi.array().items( | ||
| joi.object({ | ||
| name: joi.string(), | ||
| requiredApprovals: joi.number(), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
This is to make sure this data is propagated to the event store.
Comment on lines
323
to
341
| if ( | ||
| this.isEnterprise && | ||
| this.flagResolver.isEnabled('createProjectWithEnvironmentConfig') | ||
| ) { | ||
| if (data.changeRequestEnvironments) { | ||
| await this.validateEnvironmentsExist( | ||
| data.changeRequestEnvironments, | ||
| ); | ||
| const changeRequestEnvironments = | ||
| await enableChangeRequestsForSpecifiedEnvironments( | ||
| data.changeRequestEnvironments, | ||
| ); | ||
|
|
||
| data.changeRequestEnvironments = | ||
| changeRequestEnvironments ?? []; | ||
| } else { | ||
| data.changeRequestEnvironments = []; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the meat of the change. I've done a few things here:
- Only call this method if we're on enterprise (and if the flag is enabled)
- Validate the data in OSS before passing it to the function. That keeps all validation in one place and we can safely assume that all envs passed into the function are valid.
- Assume that the function will return the list of environments and their required approvals.
- If we're on enterprise and you didn't provide any change request environments, then we'll return an empty list to indicate that no envs were set up.
This means that when we call this in enterprise, there's less worrying about logic and that the data is valid. Instead, it's a function that takes an input and yields an output. In addition to putting all the heavy logic in one place, it also makes it easier to test all of this in one spot.
thomasheartman
changed the title
Data is typed as `any` which is *not good* and made one of the function calls be invalid
Tymek
approved these changes
Apr 25, 2024
|
Right! Can you explain what the case is; I'm not sure I understand. No environments or no change request environments? |
No environments. Not related directly to this PR |
|
Okay! Are you asking about missing |
thomasheartman
added a commit
that referenced
this pull request
Apr 29, 2024
This PR removes the workaround introduced in #6931. After ivarconr/unleash-enterprise#1268 has been merged, this should be safe to apply. Notably, this PR: - tightens up the type for the enable change request function, so we can use that to inform the code - skips trying to do anything with an empty array The last point is less important than it might seem because both the env validation and the current implementation of the callback is essentially a no-op when there are no envs. However, that's hard to enforce. If we just exit out early, then at least we know nothing happens. Optionally, we could do something like this instead, but I'm not sure it's better or worse. Happy to take input. ```ts const crEnvs = newProject.changeRequestEnvironments ?? [] await this.validateEnvironmentsExist(crEnvs.map((env) => env.name)); const changeRequestEnvironments = await enableChangeRequestsForSpecifiedEnvironments(crEnvs,); data.changeRequestEnvironments = changeRequestEnvironments; ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the handling of change request enables on project creation in two ways: