1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-07-26 13:48:33 +02:00

chore: remove workaround (#6942)

This PR removes the workaround introduced in
https://github.com/Unleash/unleash/pull/6931. After
https://github.com/ivarconr/unleash-enterprise/pull/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;
```
This commit is contained in:
Thomas Heartman 2024-04-29 13:47:47 +02:00 committed by GitHub
parent c048156e19
commit 491cd588da
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 19 additions and 21 deletions

View File

@ -50,6 +50,8 @@ describe('enterprise extension: enable change requests', () => {
const project = await service.projectStore.get(projectId);
expect(project).toBeTruthy();
return [];
},
);
});
@ -130,9 +132,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
[];
},
async () => [],
);
});
@ -215,9 +215,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
return;
},
async () => [],
);
expect(result.changeRequestEnvironments).toStrictEqual([]);
@ -245,9 +243,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
crEnvs;
},
async () => crEnvs,
);
expect('changeRequestEnvironments' in result).toBeFalsy();

View File

@ -301,9 +301,9 @@ export default class ProjectService {
enableChangeRequestsForSpecifiedEnvironments: (
environments: CreateProject['changeRequestEnvironments'],
) => Promise<
void | ProjectCreated['changeRequestEnvironments']
ProjectCreated['changeRequestEnvironments']
> = async () => {
return;
return [];
},
): Promise<ProjectCreated> {
await this.validateProjectEnvironments(newProject.environments);
@ -334,17 +334,19 @@ export default class ProjectService {
this.isEnterprise &&
this.flagResolver.isEnabled('createProjectWithEnvironmentConfig')
) {
// todo: this is a workaround for backwards compatibility
// (i.e. not breaking enterprise tests) that we can change
// once these changes have been merged and enterprise
// updated. Instead, we can exit early if there are no cr
// envs
const crEnvs = newProject.changeRequestEnvironments || [];
await this.validateEnvironmentsExist(crEnvs.map((env) => env.name));
const changeRequestEnvironments =
await enableChangeRequestsForSpecifiedEnvironments(crEnvs);
if (newProject.changeRequestEnvironments) {
await this.validateEnvironmentsExist(
newProject.changeRequestEnvironments.map((env) => env.name),
);
const changeRequestEnvironments =
await enableChangeRequestsForSpecifiedEnvironments(
newProject.changeRequestEnvironments,
);
data.changeRequestEnvironments = changeRequestEnvironments ?? [];
data.changeRequestEnvironments = changeRequestEnvironments;
} else {
data.changeRequestEnvironments = [];
}
}
await this.accessService.createDefaultProjectRoles(user, data.id);