Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Add extra configuration options to docker-registry #2801

Closed
wants to merge 7 commits into from
Closed

Add extra configuration options to docker-registry #2801

wants to merge 7 commits into from

Conversation

rendhalver
Copy link
Collaborator

I am mostly adding this so we can use it in the Portus chart I am submitting in #2766.
It will also allow people to configure their registry how they need it setup.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 20, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @rendhalver. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2017
@rendhalver rendhalver changed the title Add option to configure docker-registry with yaml [WIP] Add option to configure docker-registry with yaml Nov 20, 2017
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2017
@rendhalver rendhalver changed the title [WIP] Add option to configure docker-registry with yaml Add extra configuration options to docker-registry Nov 20, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2017
@rendhalver
Copy link
Collaborator Author

@unguiculus This will get docker-registry into a state we can configure from the portus chart in #2766.
Is there anything else I need to do to get this into stable so I can make use of it in the portus chart?

Move storage config into env vars
2223b20 
This will allow us to override that storage easier.
If this is in the configData map it will get merged into local overrides
and the container doesn't boot.
@rendhalver rendhalver mentioned this pull request Nov 27, 2017
Closed
viglesiasce
viglesiasce suggested changes Nov 27, 2017
View reviewed changes
incubator/docker-registry/values.yaml
@@ -24,3 +25,26 @@ persistentVolume:
##
storageClass: "-"
annotations: {}

fqdn: registry.docker.example.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to just be tlsSecretName or something. It is only used for that at the moment and this may be confused with a setting for ingress.

incubator/docker-registry/values.yaml
s3:
access-key: ""
secret-key: ""
configData:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add entries to the README on how these are used?

incubator/docker-registry/templates/deployment.yaml
volumeMounts:
- name: "{{ template "docker-registry.fullname" . }}-config"
Copy link
Contributor

Choose a reason for hiding this comment

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

Will TLS always be configured now? Is it worth putting this in an if block?

incubator/docker-registry/values.yaml
@@ -9,6 +9,7 @@ initImageVersion: latest
registryImage: registry
registryImageVersion: 2
tarballURL:
filesystemStorage: true
Copy link
Contributor

Choose a reason for hiding this comment

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

How are other storage systems configured? Should this be:

storage: filesystem

@rendhalver
Copy link
Collaborator Author

I guess this now needs a rebase because #2612 got merged after I opened this.

@rendhalver
Copy link
Collaborator Author

I might need to completely redo this PR.
I will have a go a rebasing it but it might be easier to chuck it out and start again.

@rendhalver rendhalver mentioned this pull request Dec 4, 2017
Merged
@rendhalver
Copy link
Collaborator Author

@viglesiasce I have redone this PR in #2926
I believe I have addressed your concerns as well.

@rendhalver rendhalver closed this Dec 4, 2017
@rendhalver rendhalver deleted the feature/docker-registry-config branch December 4, 2017 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viglesiasce viglesiasce viglesiasce requested changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rendhalver @k8s-ci-robot @viglesiasce