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

feat(cik8s,doks): add dockerhub image pull secret for datadog and jenkins-agents #2112

Merged
merged 11 commits into from
Mar 21, 2022

Conversation

smerle33
Copy link
Contributor

@smerle33 smerle33 commented Mar 21, 2022

@smerle33 smerle33 requested review from dduportal, lemeurherve and a team March 21, 2022 14:36
@smerle33 smerle33 self-assigned this Mar 21, 2022
clusters/cik8s.yaml Outdated Show resolved Hide resolved
clusters/doks.yaml Outdated Show resolved Hide resolved
clusters/ext_docker-registry-secrets.yaml Outdated Show resolved Hide resolved
clusters/ext_docker-registry-secrets.yaml Outdated Show resolved Hide resolved
timja
timja previously approved these changes Mar 21, 2022
@@ -13,7 +13,17 @@ repositories:
- name: jenkins-infra
url: https://jenkins-infra.github.io/helm-charts
releases:
- name: docker-registry-secrets
namespace: default
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't put it in the "default" namespace, maybe "docker-registry" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, but @dduportal said to go by step ;) ...
still a WIP

Copy link
Contributor

Choose a reason for hiding this comment

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

We discovered that specifying a nemspace for the helmfile release is required (despite what the doc. says) to allow referencing from other release (with the needs keyword).

Since this helm chart does not create any resource in the aformentioned namespace (it creates in the value-specified namespaceS), I proposed to use "defaukt" here, as a "no-op" value.

Stephane just added a comment in the code to explain this. WDYT?

Copy link
Member

@lemeurherve lemeurherve Mar 21, 2022

Choose a reason for hiding this comment

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

I dunno about the "default" namespace, which is frequently used for tests or install without being cautious, and subject to deleting. Not a big problem per se, but I'd put it in the existing "jenkins-agents", WDYT?

@smerle33 smerle33 marked this pull request as draft March 21, 2022 15:29
@smerle33 smerle33 marked this pull request as ready for review March 21, 2022 15:46
@dduportal dduportal enabled auto-merge (squash) March 21, 2022 15:46
@dduportal dduportal requested a review from lemeurherve March 21, 2022 15:46
@dduportal dduportal disabled auto-merge March 21, 2022 16:10
@dduportal dduportal merged commit 56bf6b1 into jenkins-infra:main Mar 21, 2022
@dduportal
Copy link
Contributor

I've merged to ensure that the credential is re-created in jenkins-agents (we had to remove it manually through the CLI to have the checks passing).

@lemeurherve if you feel like that the namespace default is not good enough, please feel free to open another PR

@smerle33 smerle33 deleted the helpdesk-2830 branch June 18, 2024 07: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.

5 participants