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

Fix order of operations when setting lease labels #7702

Merged

Conversation

austinvazquez
Copy link
Contributor

@austinvazquez austinvazquez commented Nov 19, 2022

Fixes an edge case where WithLabels would overwrite WithExpiration on a lease when using both client options and a expire value is not provided via labels. A user may expect the maps to be merged.

For example, see https://github.com/containerd/containerd/blob/main/cmd/ctr/commands/images/mount.go#L75-L81

Signed-off-by: Austin Vazquez macedonv@amazon.com

@k8s-ci-robot
Copy link

Hi @austinvazquez. Thanks for your PR.

I'm waiting for a containerd 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.

@austinvazquez austinvazquez marked this pull request as ready for review November 19, 2022 16:25
leases/lease_test.go Outdated Show resolved Hide resolved
@austinvazquez austinvazquez force-pushed the resolve-order-of-operations-in-leases branch 2 times, most recently from b0a1057 to b32a4db Compare November 22, 2022 17:16
Fixes an edge case where `WithLabels` would overwrite `WithExpiration`
on a lease when using client options.

Signed-off-by: Austin Vazquez <macedonv@amazon.com>
@austinvazquez austinvazquez force-pushed the resolve-order-of-operations-in-leases branch from b32a4db to c4dee23 Compare November 22, 2022 20:50
@dmcgowan dmcgowan merged commit 40a9464 into containerd:main Nov 30, 2022
@austinvazquez austinvazquez deleted the resolve-order-of-operations-in-leases branch December 1, 2022 21:26
@estesp estesp added cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed needs-ok-to-test labels Dec 1, 2022
@estesp
Copy link
Member

estesp commented Dec 1, 2022

Cherry-picked into release/1.5 in PR #7746
Cherry-picked into release/1.6 in PR #7745

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/1.5.x PR commits are cherry-picked into release/1.5 branch cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants