-
Notifications
You must be signed in to change notification settings - Fork 160
Add support for extra-create-metadata when creating snapshots #935
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 support for extra-create-metadata when creating snapshots #935
Conversation
|
Welcome @francoispqt! |
Hi @francoispqt. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
27d195e
to
efdfc39
Compare
/ok-to-test Thanks for working on this! Basically looks okay to me, I'll wait for it to run through tests. Remember to squash your commits before checking in. |
Thanks for reviewing! Sure I will squash my commits before merging. |
// encodeDiskTags encodes requested volume tags into JSON string, as GCE does | ||
// not support tags on GCE PDs and we use Description field as fallback. | ||
func encodeDiskTags(tags map[string]string) (string, error) { | ||
// encodeTags encodes requested volume or snapshot tags into JSON string, as GCE does |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a little out of date, as PD does support labels now. To reduce confusion, could you reword this something like the following?
enocodeTags encodes requested volume or snapshot tags into a JSON string, suitable for putting into
the PD Description field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I have updated the comment.
I think using labels would be more convenient than using the description field (for cost monitoring for example) but even though Snapshot and PD now support labels, the constraint on label values (max length 63 char) make it unsuitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Let's wait for the tests to pass, squash your commits, and I'll lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just squashed commits
cdd1673
to
8c2be6e
Compare
/lgtm Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: francoispqt, mattcary 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds metadata in the description field for Snapshots and Images created from the CreateSnapshot CSI method when the external-snapshotter is run with the
--extra-create-metadata
flag.Which issue(s) this PR fixes:
Fixes #695
Does this PR introduce a user-facing change?: