Complete source images - #866
Open
filippomc wants to merge 3 commits into
Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| domain="my.local", namespace='test', env='dev', local=False, tag=1, registry='reg') | ||
|
|
||
| helm_path = out_path / HELM_CHART_PATH | ||
| overrides = yaml.safe_load(open(helm_path / VALUES_OVERRIDES_PATH)) |
| overrides_path = helm_path / VALUES_OVERRIDES_PATH | ||
|
|
||
| # Editing the generated file is enough to change the deployed image: Helm applies it after values.yaml | ||
| overrides = yaml.safe_load(open(overrides_path)) |
| helm_path = out_path / HELM_CHART_PATH | ||
|
|
||
| # values-template-chartimages.yaml overrides only the tag: the overrides file shows the merged result | ||
| overrides = yaml.safe_load(open(helm_path / VALUES_OVERRIDES_PATH)) |
| out_path = tmp_path / 'test_values_overrides_no_include' | ||
| create_helm_chart([CLOUDHARNESS_ROOT, RESOURCES], output_path=out_path, domain="my.local", | ||
| namespace='test', env='dev', local=False, tag=1) | ||
| overrides = yaml.safe_load(open(out_path / HELM_CHART_PATH / VALUES_OVERRIDES_PATH)) |
| # An application declaring a prebuilt image is not built, so that image IS an overridable | ||
| # source, unlike the image CloudHarness would have built for it | ||
| assert values[KEY_APPS]['myapp']['build'] is False | ||
| overrides = yaml.safe_load(open(out_path / HELM_CHART_PATH / VALUES_OVERRIDES_PATH)) |
aranega
approved these changes
Sep 3, 2026
aranega
left a comment
aranega
left a comment
Member
There was a problem hiding this comment.
The PR looks good to me, I just added a comment/nitpick about the fact of using some hardcoded path, but I'm unsure the line in question is something which is supposed to run on the CI/CD or on the user's machine.
| arguments = deployment_step.get("arguments") | ||
| if arguments: | ||
| # Apply the generated image overrides after values.yaml so editing them wins | ||
| values_overrides_file = f"./{DEPLOYMENT_PATH}/{HELM_CHART_PATH}/{VALUES_OVERRIDES_PATH}" |
Member
There was a problem hiding this comment.
nitpick: it could be cool to use more Path object from the pathlib builtin libs as it removes the use of / as os dependent separator. This is something which is not entirely coherent through our base code (CH-269 points to this as something to solve), but in some cases, I'm not sure if that's meant to be run only on the CI/CD (usually linux-based) or on the user machine.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes CH-287
Implemented solution
All image paths/values are exported as values-overrides.yaml, so it's easier to change everything in one place.
harness-deploymentgenerate that file with every image that's relevantHow to test this PR
Run
harness-deploymentand then checkdeployment/helm/values-overrides.yaml.It should contain all the relevant source images.
Sanity checks:
Breaking changes (select one):
breaking-changeand the migration procedure is well described abovePossible deployment updates issues (select one):
alert:deploymentTest coverage (select one):
Documentation (select one):
Nice to have (if relevant):