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

Fix component multi-version verification #1100

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Conversation

simu
Copy link
Member

@simu simu commented Feb 7, 2025

We need to check whether a component instance that sets a custom version supports per-instance versions by checking the instance's target's parameters. The bootstrap target ("cluster") doesn't have any instance-specific parameters, but since we do the validation after setting up all instance targets, we can use the instance's target to read _metadata of the alias's version to properly validate that both the base component and the alias are multi-version aware.

Additionally, because component compile doesn't have the regular bootstrap target (and generating it just for alias validation seems a bit overkill), we make the bootstrap_target key an optional parameter for verify_component_aliases() and set it to the component that's being compiled for component compile and fall back to config.inventory.boostrap_target when it's not set.

We also adjust all tests that cover verify_component_aliases() to pass a Kapitan/reclass inventory dict instead of just a parameters dict and update the test inputs to set the alias _metadata in the component parameters key instead of the instance parameters key to replicate the structure that we generate for catalog compile.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update tests.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency, internal
    as they show up in the changelog
  • Link this PR to related issues.

@simu simu added the bug Something isn't working label Feb 7, 2025
simu added 2 commits February 7, 2025 11:53
We need to check whether a component instance that sets a custom version
supports per-instance versions by checking the instance's target's
parameters. The bootstrap target ("cluster") doesn't have any
instance-specific parameters, but since we do the validation after
setting up all instance targets, we can use the instance's target to
read `_metadata` of the alias's version to properly validate that both
the base component and the alias are multi-version aware.

Additionally, because `component compile` doesn't have the regular
bootstrap target (and generating it just for alias validating seems a
bit overkill), we make the `bootstrap_target` key an optional parameter
for `verify_component_aliases()` and set it to the component that's
being compiled for `component compile` and fall back to
`config.inventory.boostrap_target` when it's not set.
Adjust all tests that cover `verify_component_aliases()` to pass a
Kapitan/reclass inventory dict instead of just a parameters dict.

Additionally, update test inputs to set the alias `_metadata` in the
component parameters key instead of the instance parameters key to
replicate the structure that we generate for `catalog compile`.
@simu simu force-pushed the fix/multi-version-verification branch from 906c448 to d2c548e Compare February 7, 2025 10:57
@simu simu marked this pull request as ready for review February 7, 2025 10:59
@simu simu requested a review from a team as a code owner February 7, 2025 11:00
@simu simu requested a review from a team February 7, 2025 11:00
Copy link
Member

@haasad haasad left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with the code base, but the changes makes sense and match what is stated in the PR description

@simu simu merged commit 6217d08 into master Feb 12, 2025
20 checks passed
@simu simu deleted the fix/multi-version-verification branch February 12, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants