fix: ensure NillableDuration is round-trippable #1545

jmdeal · 2024-08-08T19:32:45Z

Fixes #N/A

Description
Ensure our NillableDuration type is round-trippable for conversion webhooks.

How was this change tested?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pkg/apis/v1/duration.go

coveralls · 2024-08-09T17:38:31Z

Pull Request Test Coverage Report for Build 10323210198

Details

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.02%) to 77.875%

Files with Coverage Reduction	New Missed Lines	%
pkg/test/expectations/expectations.go	2	94.64%
pkg/controllers/disruption/drift.go	2	89.66%
pkg/controllers/node/termination/controller.go	2	66.67%

Totals
Change from base Build 10322150232:	-0.02%
Covered Lines:	8891
Relevant Lines:	11417

💛 - Coveralls

pkg/apis/v1/nodepool_conversion_test.go

jonathan-innis · 2024-08-11T21:14:09Z

pkg/apis/v1/nodepool_conversion_test.go

+				duration := NillableDuration{}
+				Expect(json.Unmarshal([]byte(str), &duration)).Should(Succeed())
+				*field() = duration
+				Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())


Just validating: This test failed prior to this change?

Also, any reason to not json marshal the entire object rather than just the specific field that we care about during the test? Seems like a stronger bar to hold if we do that

Yep, this failed before. I was only marshaling this as json to ensure we populated the "raw" value correctly before doing the conversions, I've updated every instance of us creating a NillableDuration with a constructor that covers this instead.

njtran

one comment but LGTM

pkg/apis/v1/nodepool_conversion_test.go

njtran

/lgtm
/approve

k8s-ci-robot · 2024-08-12T01:29:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmdeal, njtran

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:

~~OWNERS~~ [njtran]

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

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

k8s-ci-robot requested review from engedaam and jackfrancis August 8, 2024 19:32

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 8, 2024

jmdeal changed the title ~~checkpoint~~ fix: ensure NillableDuration is round-trippable Aug 8, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2024

rschalo reviewed Aug 8, 2024

View reviewed changes

pkg/apis/v1/duration.go Outdated Show resolved Hide resolved

jmdeal force-pushed the fix/round-trip-duration branch 2 times, most recently from 0c9cc81 to d704fb2 Compare August 8, 2024 20:22

jmdeal changed the title ~~fix: ensure NillableDuration is round-trippable~~ [DRAFT] fix: ensure NillableDuration is round-trippable Aug 9, 2024

jmdeal force-pushed the fix/round-trip-duration branch from d704fb2 to 08b1580 Compare August 9, 2024 17:26

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2024

jmdeal force-pushed the fix/round-trip-duration branch from 08b1580 to de4365f Compare August 9, 2024 19:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 9, 2024

jmdeal force-pushed the fix/round-trip-duration branch 2 times, most recently from 14e0a2b to 15e5a6c Compare August 9, 2024 20:19

fix: ensure NillableDuration is round-trippable

97bb0cd

jmdeal force-pushed the fix/round-trip-duration branch from 15e5a6c to 97bb0cd Compare August 9, 2024 22:42

jmdeal changed the title ~~[DRAFT] fix: ensure NillableDuration is round-trippable~~ fix: ensure NillableDuration is round-trippable Aug 9, 2024

njtran reviewed Aug 10, 2024

View reviewed changes

pkg/apis/v1/nodepool_conversion_test.go Outdated Show resolved Hide resolved

pkg/apis/v1/nodepool_conversion_test.go Outdated Show resolved Hide resolved

pkg/apis/v1/nodepool_conversion_test.go Outdated Show resolved Hide resolved

jonathan-innis reviewed Aug 11, 2024

View reviewed changes

jmdeal force-pushed the fix/round-trip-duration branch 4 times, most recently from b67ec92 to 324c516 Compare August 11, 2024 23:31

This was referenced Aug 11, 2024

chore: Backport conversion webhooks changes for v0.33.x #1549

Merged

chore: Backport conversion webhooks changes for v0.34.x #1550

Merged

chore: Backport conversion webhooks changes for v0.35.x #1551

Merged

jmdeal force-pushed the fix/round-trip-duration branch from 46a825a to dda7da2 Compare August 12, 2024 00:55

added constructor

7527dc9

jmdeal force-pushed the fix/round-trip-duration branch from dda7da2 to 7527dc9 Compare August 12, 2024 00:56

njtran reviewed Aug 12, 2024

View reviewed changes

pkg/apis/v1/nodepool_conversion_test.go Outdated Show resolved Hide resolved

Update pkg/apis/v1/nodepool_conversion_test.go

7fa6a86

njtran reviewed Aug 12, 2024

View reviewed changes

k8s-ci-robot assigned njtran Aug 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 12, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2024

k8s-ci-robot merged commit feb6857 into kubernetes-sigs:main Aug 12, 2024
10 of 11 checks passed

jmdeal mentioned this pull request Aug 12, 2024

deps: bump sigs.k8s.io/karpenter aws/karpenter-provider-aws#6705

Merged

3 tasks

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

e3b8488

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

571e700

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam mentioned this pull request Aug 12, 2024

chore: Backport NillableDuration updates for v0.35.x #1554

Merged

rschalo mentioned this pull request Aug 12, 2024

chore: Backport NillableDuration updates for v0.37.x #1555

Merged

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

d69b715

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

77cc14f

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam mentioned this pull request Aug 12, 2024

chore: Backport NillableDuration updates for v0.36.x #1556

Merged

rschalo pushed a commit to rschalo/karpenter that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

2ffc567

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

046c6b0

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

rschalo mentioned this pull request Aug 12, 2024

chore: Backport NillableDuration updates for v0.33.x #1557

Merged

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

d374550

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

100d7ed

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

98c9a44

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

rschalo pushed a commit to rschalo/karpenter that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

1a0f61e

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

rschalo mentioned this pull request Aug 12, 2024

chore: Backport NillableDuration updates for v0.34.x #1558

Merged

engedaam pushed a commit to engedaam/karpenter-core that referenced this pull request Aug 12, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

107de39

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

jmdeal deleted the fix/round-trip-duration branch August 14, 2024 22:55

BEvgeniyS pushed a commit to BEvgeniyS/karpenter that referenced this pull request Sep 16, 2024

fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

b1fadc3

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ensure NillableDuration is round-trippable #1545

fix: ensure NillableDuration is round-trippable #1545

jmdeal commented Aug 8, 2024

coveralls commented Aug 9, 2024

jonathan-innis Aug 11, 2024

jonathan-innis Aug 11, 2024

jmdeal Aug 11, 2024 •

edited

Loading

njtran left a comment

njtran left a comment

k8s-ci-robot commented Aug 12, 2024

fix: ensure NillableDuration is round-trippable #1545

fix: ensure NillableDuration is round-trippable #1545

Conversation

jmdeal commented Aug 8, 2024

coveralls commented Aug 9, 2024

Pull Request Test Coverage Report for Build 10323210198

Details

💛 - Coveralls

jonathan-innis Aug 11, 2024

Choose a reason for hiding this comment

jonathan-innis Aug 11, 2024

Choose a reason for hiding this comment

jmdeal Aug 11, 2024 • edited Loading

Choose a reason for hiding this comment

njtran left a comment

Choose a reason for hiding this comment

njtran left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 12, 2024

jmdeal Aug 11, 2024 •

edited

Loading