Skip to content

Commit

Permalink
chore: remove workaround (#6942)
Browse files Browse the repository at this point in the history
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;
```
  • Loading branch information
thomasheartman authored Apr 29, 2024
1 parent c048156 commit 491cd58
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 22 deletions.
14 changes: 5 additions & 9 deletions src/lib/features/project/project-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ describe('enterprise extension: enable change requests', () => {
const project = await service.projectStore.get(projectId);

expect(project).toBeTruthy();

return [];
},
);
});
Expand Down Expand Up @@ -130,9 +132,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
[];
},
async () => [],
);
});

Expand Down Expand Up @@ -215,9 +215,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
return;
},
async () => [],
);

expect(result.changeRequestEnvironments).toStrictEqual([]);
Expand Down Expand Up @@ -245,9 +243,7 @@ describe('enterprise extension: enable change requests', () => {
isAPI: false,
},
TEST_AUDIT_USER,
async () => {
crEnvs;
},
async () => crEnvs,
);

expect('changeRequestEnvironments' in result).toBeFalsy();
Expand Down
28 changes: 15 additions & 13 deletions src/lib/features/project/project-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);

data.changeRequestEnvironments = changeRequestEnvironments ?? [];
if (newProject.changeRequestEnvironments) {
await this.validateEnvironmentsExist(
newProject.changeRequestEnvironments.map((env) => env.name),
);
const changeRequestEnvironments =
await enableChangeRequestsForSpecifiedEnvironments(
newProject.changeRequestEnvironments,
);

data.changeRequestEnvironments = changeRequestEnvironments;
} else {
data.changeRequestEnvironments = [];
}
}

await this.accessService.createDefaultProjectRoles(user, data.id);
Expand Down

0 comments on commit 491cd58

Please sign in to comment.