-
Notifications
You must be signed in to change notification settings - Fork 34
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: enable setting catalog version without "v" prefix #357
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ksimon1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
scripts/ansible/common.yaml
Outdated
@@ -13,6 +13,8 @@ sa_name: "{{ role_name }}" | |||
role_binding_name: "{{ role_name }}" | |||
default_file_mode: "0644" | |||
version: "{{ lookup('env','RELEASE_VERSION')| default('latest', true) }}" | |||
remove_catalog_version_prefix: "{{ lookup('env','REMOVE_CATALOG_VERSION_PREFIX')| default(false, true) | bool}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove_catalog_version_prefix: "{{ lookup('env','REMOVE_CATALOG_VERSION_PREFIX')| default(false, true) | bool}}" | |
remove_catalog_version_prefix: "{{ lookup('env','REMOVE_CATALOG_VERSION_PREFIX')| default(false) | bool}}" |
Shouldn't it look like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to docs (https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_filters.html#providing-default-values)
If you want to use the default value when variables evaluate to false or an empty string you have to set the second parameter to true:
So I assume the true can stay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're both right in this specific case. When using bools we don't care for empty vars as they evaluate to false too. But in other cases your solution might be preferred!
this commit allows to set REMOVE_CATALOG_VERSION_PREFIX env variable, which will remove "v" prefix from version in catalog version - e.g. 0.19.0 when true vs v0.19.0 when false. This will help in DS where OCP catalog removes v from version. This commit also replaces all version variables from catalog version and replaces it with catalog_version which can hold version with or without v prefix Signed-off-by: Karel Simon <[email protected]>
/retest |
/lgtm |
/retest |
What this PR does / why we need it:
chore: enable setting catalog version without "v" prefix
this commit allows to set REMOVE_CATALOG_VERSION_PREFIX env variable, which will remove "v" prefix from version in catalog version - e.g. 0.19.0 when true vs v0.19.0 when false.
This will help in DS where OCP catalog removes v from version. This commit also replaces all version variables from catalog version and replaces it with catalog_version which can hold version with or without v prefix
Signed-off-by: Karel Simon [email protected]
Release note: