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 issue that doesn't allow using upstream with match_if_app_name_starts_with set to true in copy-image-from-upstream command #136

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

rafaelgomesxyz
Copy link
Collaborator

@rafaelgomesxyz rafaelgomesxyz commented Jan 19, 2024

NOTE: On top of #135

The copy-image-from-upstream command does not currently allow using an app that has match_if_app_name_starts_with set to true as the upstream.

This PR fixes that.

@rafaelgomesxyz rafaelgomesxyz force-pushed the copy-image-from-upstream-starts-with-fix branch from 8793caa to d1fd881 Compare January 19, 2024 14:52
@@ -102,6 +102,13 @@ def current
end
end

def find_app_config(app_name1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just made it public to be able to use it.

@@ -29,7 +29,7 @@ def call # rubocop:disable Metrics/MethodLength
ensure_docker_running!

@upstream = config[:upstream]
@upstream_org = config.apps[@upstream.to_sym][:cpln_org] || ENV.fetch("CPLN_ORG_UPSTREAM", nil)
@upstream_org = config.find_app_config(@upstream)&.dig(:cpln_org) || ENV.fetch("CPLN_ORG_UPSTREAM", nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's say we have a dummy-test app in controlplane.yml that has match_if_app_name_starts_with set to true.

The old method wouldn't work if called with, e.g., dummy-test-123, because config.apps only contains the apps exactly as they are in controlplane.yml, and we have dummy-test in controlplane.yml, not dummy-test-123.

The new method works by iterating through config.apps looking for an app that matches dummy-test-123, so it will find dummy-test and match it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we'd use a starts with for the upstream. That should be a validation error. Agree, @dzirtusss

Base automatically changed from copy-image-from-upstream-profile-fix to main January 22, 2024 03:51
@justin808 justin808 merged commit 93fa1ee into main Jan 22, 2024
3 checks passed
@justin808 justin808 deleted the copy-image-from-upstream-starts-with-fix branch January 22, 2024 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants