[CLOUDP-352383] Add a build variant to release chart to OCI registry #542
Conversation
viveksinghggits
added
the
skip-changelog
MCK 1.6.0 Release NotesNew Features
Bug Fixes
|
|
@MaciejKaras @lucian-tosa can you please have a look into this PR as well. |
| raise Exception(f"Unable to load Chart.yaml from dir {chart_path}") | ||
|
|
||
| # if build_scenario is release, the chart.yaml would already have correct chart version | ||
| if build_scenario == BUILD_SCENARIO_RELEASE: |
There was a problem hiding this comment.
why not hide this logic in build_info.json? You could specify for example new field version_prefix as 0.0.0+ in patch and staging build scenarios. If it is specified just concatenate it with actual version_id, otherwise use plain version_id
There was a problem hiding this comment.
So, there are quite a few things here. First is, this change of figuring out the version of dev/staging is not really part of this PR, it was merged as part of the change in this PR.
Having said that, I can make the change (in new PR) but I would like to understand the benefit of doing that. According to me if we have the build_scenario and with that if we can figure something out without looking into build_info.json, does it make sense to look into build_info.json?
Having the eventual version dependent on build scenario makes the code more readable, if it's dev/staging we are appending 0.0.0. But if we read the prefix from build_info, like you said we are hiding away why we are appending things based on prefix.
There was a problem hiding this comment.
Or in other words, does it make sense to make something configurable, which doesn't need to be. We know that we are not going to change 0.0.0 to something else.
There was a problem hiding this comment.
According to me if we have the build_scenario and with that if we can figure something out without looking into build_info.json, does it make sense to look into build_info.json?
If we follow this logic trail, we could just have the build_scenario variable and figure out everything in the code and don't read configuration in build_info.json at all. The reason for having build_info.json is to be able to configure different behaviour for different build scenarios and not have conditional logic in the code. Additional benefit is that we can read the build_info.json configuration and know how it is configured for each build scenario.
There was a problem hiding this comment.
Having the eventual version dependent on build scenario makes the code more readable, if it's dev/staging we are appending 0.0.0. But if we read the prefix from build_info, like you said we are hiding away why we are appending things based on prefix.
I think we can still add comment about this in build_info.json next to dev and staging scenarios.
There was a problem hiding this comment.
Or in other words, does it make sense to make something configurable, which doesn't need to be. We know that we are not going to change 0.0.0 to something else.
That's true, to build configuration will rarely change, but it doesn't mean this is the only benefit of having it - to change the configuration. The biggest gain in my opinion is to move abstract conditional logic from the code and make it simple, readable and testable.
There was a problem hiding this comment.
ok, I can make the change. Is it ok if I do it in the new PR?
The reason is, I want to focus one PR in just one change, don't want to do multiple changes in single PR.
There was a problem hiding this comment.
Sure, it can be done separately.
![semgrep-code-mongodb[bot]](https://avatars.githubusercontent.com/u/45120?s=60&v=4){kind=small}
3 participants
Summary
In one of the previous PRs we made sure that we are publishing our helm chart to OCI container registry, as part of the work where we are going to eventually move to the OCI container registry for our helm chart repo.
After the chart is being published to to dev/staging registries, this PR makes sure that the chart get released to quay, as part of our release process.
We will keep publishing helm chart to the old github repo and quay container registry, for some time and will eventually move to just publishing it to OCI.
Proof of Work
This evg patch is for dev scenario: https://spruce.mongodb.com/version/68f74e0d5420540007c32361/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC
This one is for release, and we were able to publish to quay successfully: https://spruce.mongodb.com/version/68f75417f6cd5700079c0cb9/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC
Checklist
skip-changeloglabel if not needed