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

Feature: Add registry override variable support, and add additional messaging around this in logs #132

Merged
merged 9 commits into from
Nov 1, 2022

Conversation

Schnitzel
Copy link
Contributor

this allows to override an existing password in the .lagoon.yml

@Schnitzel
Copy link
Contributor Author

@shreddedbacon @tobybellwood would love your feedback on this. I personally feel this is a good thing as in the very beginning I think we did not have the possibility to add the password to the API and so there might be lagoon users out there that have the password hardcoded, Having a default say to search for a password in the API in this form: ${PRIVATE_CONTAINER_REGISTRY}_PASSWORD might not be a bad practice?

@shreddedbacon
Copy link
Member

I like the idea, but I think using ${PRIVATE_CONTAINER_REGISTRY}_PASSWORD might not be ideal. If you call your registry test in the .lagoon.yml. You create test_PASSWORD variable in the API under container_registry scope. But if you use a TEST_PASSWORD variable in the project already, you won't be able to add the container registry override, as we constrain name/project as unique key, even though the scopes would be different, and the case is different.

$ lagoon add variable -p example-project -N test_PASSWORD -V mypassword -S container_registry
Result: success
Project: example-project
ID: 4

$ lagoon add variable -p example-project -N TEST_PASSWORD -V myotherpassword -S global
Error: graphql: (conn=204366, no: 1062, SQLState: 23000) Duplicate entry 'TEST_PASSWORD-1' for key 'name_project'
sql: insert into `env_vars` (`environment`, `id`, `name`, `project`, `scope`, `value`) values (DEFAULT, DEFAULT, 'TEST_PASSWORD', 1, 'global', 'myotherpassword') - parameters:{}

Might be better to use REGISTRY_${PRIVATE_CONTAINER_REGISTRY}_PASSWORD to make it clear it is a registry password, and then the likely hood of potential clashes is lower

I know for the particular customer impacted by this problem it won't matter if the REGISTRY_ prefix was there, but just thinking about others potential users

@shreddedbacon
Copy link
Member

Closing in favor of #133

@shreddedbacon
Copy link
Member

Reopening, #133 contains other PRs not confirmed for merging yet

@shreddedbacon shreddedbacon reopened this Oct 27, 2022
@shreddedbacon shreddedbacon changed the title New ${PRIVATE_CONTAINER_REGISTRY}_PASSWORD variable in API Feature: Add registry override variable support, and add additional messaging around this in logs Oct 27, 2022
@shreddedbacon
Copy link
Member

This has been running for one of our customers for some time now with no issues.

@shreddedbacon shreddedbacon merged commit a12604c into main Nov 1, 2022
@shreddedbacon shreddedbacon deleted the container-registry-password-override branch November 1, 2022 23:24
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