feat(manifest): Loaded manifest includes all environment and group names #1897
Conversation
This file contains hidden or 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
arthurpitman
added
the
run-e2e-test
Kirdock
reviewed
May 19, 2025
| errs = append(errs, manifestErrors...) | ||
| } else if len(environmentDefinitions) == 0 { | ||
| } else if len(environments.AllEnvironmentNames) == 0 { |
There was a problem hiding this comment.
So this is the only behavior change, right?
There was a problem hiding this comment.
With this new data structure, I think this is the most robust way to test for this condition.
Kirdock
approved these changes
May 19, 2025
tomazpu
approved these changes
May 20, 2025
arthurpitman
force-pushed
the
feat/loaded-manifest-includes-all-environment-and-group-names
branch
from
f52f757 to
f802337
Compare
…yName` `Environments` is not a great name doesn't add value compared to `map[string]EnvironmentDefinition`, but allows us to define `(e EnvironmentDefinitionsByName) Names()`. In most cases, this method also brings no value, as iterating over the map gives us the names anyway. But it simplifies some tests, as then no actual `EnvironmentDefinition` objects are required. By renaming, we can reuse the name `Environments` in the next commit.
…ectedEnvironments` This makes it clear what is contained here: the subset of environments from the manifest actually selected for use.
…the manifest This type includes the selected `EnvironmentDefinitions` as well as the names of all environments and groups.
arthurpitman
force-pushed
the
feat/loaded-manifest-includes-all-environment-and-group-names
branch
from
f802337 to
9d2ce35
Compare
|
Laubi
approved these changes
May 21, 2025
arthurpitman
deleted the
feat/loaded-manifest-includes-all-environment-and-group-names
branch
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.
Why this PR?
This PR is an alternative to #1876 and this time adds all environment and environment group names seen during loading directly to the manifest data structure.
This PR is more robust because
manifest.Environments.SelectedEnvironmentsis identical to the currentmanifest.Environments, meaning it avoids the effort of having to explicitly deal withSkip. However, a lot of effort had to be invested in updating the tests.What has changed?
The
Environmentsfield ofmanifest.Manifestis now a struct of typemanifest.Environmentsthat holds selected environments as well as the names of all environments and environment groups seen during loading. This extra information will be used #1880 to emit a warning if an undefined environment or group is referenced.How does it do it?
The PR does some renaming and then simply adds the new type and updates tests. The actual change to production code is pretty small.
Best reviewed commit by commit.
How is it tested?
Existing tests are updated to include the new data structure, which holds information about the names of all environments and environment groups.
How does it affect users?
No effect on users.