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

Override Docker download URL #6956

Closed

Conversation

austinmoore-
Copy link
Contributor

Allows users to specify where to download Docker with a new spec.dockerInstall.overrideSources spec. See the added docs for details on how to use it.

This is effectively a way to override the built in dockerVersions slice and specify your own. This provides a nice escape hatch if you cannot download from external repos and must use internal mirrors (as in my company's situation). Ideally this would just be a url and hash. However, some packages have dependencies that need to be specified and other packages need to be installed simultaneously with other packages.

There are some considerations to keep in mind with this feature. This solution isn't the most intuitive for a user because the user will probably just end up looking at dockerVersions to find their OS and Docker version. If the user changes their OS or Docker version, this might cause problems if they forget to update this configuration. I'm open to ideas on how to handle these situations.

I think this might be a stop-gap measure for a more robust solution. For example, in the future it might be better to provide a way to override the base download URL like we do for some other resources. I didn't do this because I figured it was better to expose full control for something like this. I also wasn't sure if we need to consider mirrored repos that do not have the same path (not sure how common this would be).

Another option could be to allow a way to externalize the data in dockerVersions to a YAML file in S3 as part of the cluster that the user could then edit or specify.

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

Hi @austinmoore-. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 13, 2019
@austinmoore-
Copy link
Contributor Author

/assign @justinsb

@michaelajr
Copy link

Hi. Hoping this can get looked at, tested, and merged. We really need this as we can not download Docker from the public repo. Thanks!

@selslack
Copy link

@justinsb @robinpercy @rdrgmnzs please take a look at the PR.

@Nitecon
Copy link

Nitecon commented Aug 13, 2019

This looks good and would be nice to have!

@rdrgmnzs
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 13, 2019
@selslack
Copy link

selslack commented Sep 10, 2019

@justinsb @robinpercy @rdrgmnzs is there anything needs to be done from the community to get this PR merged and back ported to 1.13 and/or 1.14 if the release is really soon?

@austinmoore-
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: austinmoore-
To complete the pull request process, please assign justinsb
You can assign the PR to them by writing /assign @justinsb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@austinmoore-
Copy link
Contributor Author

Removed DockerInstallSpec per this discussion.

@austinmoore-
Copy link
Contributor Author

/test pull-kops-e2e-kubernetes-aws

@michaelajr
Copy link

Any chance this will make it into the next release? @rdrgmnzs

@austinmoore-
Copy link
Contributor Author

Rebased on to master

@austinmoore-
Copy link
Contributor Author

/test pull-kops-verify-generated

@austinmoore-
Copy link
Contributor Author

Rebased onto master

@michaelajr
Copy link

michaelajr commented Oct 14, 2019

@rdrgmnzs @robinpercy hoping to get this merged for the next release. Can you (or someone) take a look? This is a needed feature for my workplace.

@austinmoore-
Copy link
Contributor Author

/hold

Reconsidering this, because it doesn't handle instance groups

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019
@austinmoore-
Copy link
Contributor Author

Closing in favor of #7974.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants