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

Add "kops get assets" command #11617

Merged
merged 6 commits into from
May 30, 2021
Merged

Conversation

johngmyers
Copy link
Member

This is rough around the edges, but gives the idea.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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. labels May 28, 2021
@k8s-ci-robot k8s-ci-robot added area/documentation needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 28, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2021
@johngmyers johngmyers force-pushed the assets branch 2 times, most recently from 6d66218 to bd5a8c0 Compare May 29, 2021 04:18
@johngmyers johngmyers changed the title WIP Add "kops get assets" command Add "kops get assets" command May 29, 2021
@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 May 29, 2021
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Small suggestion, maybe skip the mirror field when not needed.
Also, would be nice to move this to klog.V(1).Info():

I0529 08:59:26.979910   39040 dns.go:97] Private DNS: skipping DNS validation

Comment on lines 113 to 114
Mirror: containerAsset.DockerImage,
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mirror: containerAsset.DockerImage,
}
}
if containerAsset.DockerImage != containerAsset.CanonicalLocation {
image.Mirror = containerAsset.DockerImage
}

Comment on lines 124 to 126
Mirror: fileAsset.DownloadURL.String(),
SHA: fileAsset.SHAValue,
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mirror: fileAsset.DownloadURL.String(),
SHA: fileAsset.SHAValue,
}
SHA: fileAsset.SHAValue,
}
if fileAsset.DownloadURL.String() != fileAsset.CanonicalURL.String() {
file.Mirror = fileAsset.DownloadURL.String()
}


type Image struct {
Image string `json:"image"`
Mirror string `json:"mirror"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mirror string `json:"mirror"`
Mirror string `json:"mirror,omitempty"`


type File struct {
File string `json:"file"`
Mirror string `json:"mirror"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Mirror string `json:"mirror"`
Mirror string `json:"mirror,omitempty"`

@johngmyers
Copy link
Member Author

I support suppressing chatter like that DNS log line, though I think that's perhaps a different PR.

On the mirror field, I'm a little wary of requiring consumers of that data to implement special cases for edge cases like this.
Perhaps I'm using the wrong terminology? Should I stick with "Download" and "Canonical"?

I could see a desire for less information when downloading from the canonical location, but then I don't see any use cases for this command in that situation.

@hakman
Copy link
Member

hakman commented May 30, 2021

I understood perfectly the terminology, no worries. Though the name of the field "mirror" is misleading. This reminds me of the mirrored assets we already have.
One can use this to inspect what assets will be downloaded in advance, for example to see what CNI or addon versions will be used by default. Another use case (more popular these days) is for building AMIs with these assets pre-loaded.

@johngmyers
Copy link
Member Author

@hakman the concern wasn't about understanding, but with which terminology was appropriate for the feature. I've changed it to be more consistent and accurate about what the values are.

Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Agreed. Thanks!
Feel free to cherry-pick to any release.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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

The pull request process is described 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2021
@k8s-ci-robot k8s-ci-robot merged commit 02034d0 into kubernetes:master May 30, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 30, 2021
@johngmyers johngmyers deleted the assets branch May 30, 2021 18:08
@johngmyers
Copy link
Member Author

It's a bit late to add features to 1.21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

None yet

3 participants