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

chore: remove workaround #6942

Merged
merged 2 commits into from
Apr 29, 2024
Merged

chore: remove workaround #6942

merged 2 commits into from
Apr 29, 2024

Conversation

thomasheartman
Copy link
Contributor

@thomasheartman thomasheartman commented Apr 26, 2024

This PR removes the workaround introduced in #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.

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

            data.changeRequestEnvironments = changeRequestEnvironments;

Copy link

vercel bot commented Apr 26, 2024

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 Apr 26, 2024 5:41am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Apr 26, 2024 5:41am

Comment on lines +327 to +339
if (newProject.changeRequestEnvironments) {
await this.validateEnvironmentsExist(
newProject.changeRequestEnvironments.map((env) => env.name),
);
const changeRequestEnvironments =
await enableChangeRequestsForSpecifiedEnvironments(
newProject.changeRequestEnvironments,
);

data.changeRequestEnvironments = changeRequestEnvironments;
} else {
data.changeRequestEnvironments = [];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the description, this could be this instead (essentially the same as before, but without the comment and without extra set of ??s. Happy to hear your thoughts.

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

I kinda like this version, because it avoids all the nesting and it reads cleaner to me 🤷🏼

Copy link
Member

Choose a reason for hiding this comment

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

More verbose version looks more readable

Comment on lines +327 to +339
if (newProject.changeRequestEnvironments) {
await this.validateEnvironmentsExist(
newProject.changeRequestEnvironments.map((env) => env.name),
);
const changeRequestEnvironments =
await enableChangeRequestsForSpecifiedEnvironments(
newProject.changeRequestEnvironments,
);

data.changeRequestEnvironments = changeRequestEnvironments;
} else {
data.changeRequestEnvironments = [];
}
Copy link
Member

Choose a reason for hiding this comment

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

More verbose version looks more readable

@thomasheartman thomasheartman merged commit 491cd58 into main Apr 29, 2024
7 checks passed
@thomasheartman thomasheartman deleted the chore/remove-workaround branch April 29, 2024 11:47
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