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

Added release builder #2020

Closed

Conversation

micahhausler
Copy link
Member

The release builder fetches pre-built Kubernetes artifacts for a node image from a standard upstream release URL.

Reviewer notes:

  • I modified the New{Docker,Bazel,Release}Builder() signature to include the releaseUrl parameter. This parameter is unused in Docker/Bazel, but the kubeRoot param is unused in the ReleaseBuilder. I did this to simplify the diff so that nameToImpl could just add "release" as one more item in the switch statement. Let me know if a larger refactor is preferred.
  • I did not add any tests, as the rest of the package doesn't include any. I'd be happy to add tests if desired

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: micahhausler
To complete the pull request process, please assign munnerz after the PR has been reviewed.
You can assign the PR to them by writing /assign @munnerz 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

@k8s-ci-robot k8s-ci-robot requested review from amwat and munnerz January 21, 2021 23:54
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 21, 2021
@k8s-ci-robot
Copy link
Contributor

@micahhausler: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kind-verify 6553e03 link /test pull-kind-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@BenTheElder
Copy link
Member

First: thanks for the PR! 😄


I modified the New{Docker,Bazel,Release}Builder() signature to include the releaseUrl parameter. This parameter is unused in Docker/Bazel, but the kubeRoot param is unused in the ReleaseBuilder. I did this to simplify the diff so that nameToImpl could just add "release" as one more item in the switch statement. Let me know if a larger refactor is preferred.

I've been thinking about this for v0.11.0 and I think we want to support this but not quite like this:

  • I think we should deprecate the --kube-root flag and take the source spec as an argument instead which seems more natural and can be passed to any of the builders (also we prefer not to have disjoint flags in the CLI, you won't really find any currently)
  • We should be considering multi-platform (can wait until after this PR)
  • We should support another kind of spec that allows more control over the node image than just using a release URL, I think a small YAML spec specifing the sources for built-in manifests, images, etc., I've been drafting a design for this.

Most of these are not a blocker for adding this, but I think instead of adding a new flag we should go ahead and deprecate kube-root in favor of passing through the arguments to leave room for these additional mode without a proliferation of mode-specific flags.

Most of the modes should be able to translate a string specifying the source location (kubernetes source dir, yaml nodeImage spec filepath, or remote release URI), with some code to detect which the argument is.

The flag controlling build mode for source builds can go away with kubernetes/kubernetes#88553 (I haven't mentioned this yet, but I'm actively drafting a concrete proposal for this before circling back to #166 / #381 ...)

aside: we prefer to discuss design for features in issues before moving to pull requests, check out our detailed new contributor guide for this and more 🙃 https://kind.sigs.k8s.io/docs/contributing/getting-started/

I did not add any tests, as the rest of the package doesn't include any. I'd be happy to add tests if desired

I'm not sure how we want to approach this, we've been able to get pretty good coverage of these because we build from source in CI, I think perhaps at least one of the github actions jobs should move to using this build from release to ensure coverage.

@BenTheElder
Copy link
Member

this currently fails make verify which will need to be fixed. https://kind.sigs.k8s.io/docs/contributing/development/#linting

@micahhausler
Copy link
Member Author

I'm happy to close this out and come up with a better design first. This was a quick POC, and I'm glad to hear you're already thinking about grabbing pre-built artifacts from somewhere else.

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants