changes to update resources based on predetermined interval #571

kumaritanushree · 2022-08-04T08:55:31Z

What this PR does / why we need it:

Kapp will update resources based on predetermined interval set. To set the interval user needs to add annotation kapp.k14s.io/renew-duration to the resource

Which issue(s) this PR fixes:

Fixes #224

Does this PR introduce a user-facing change?

New annotation has been introduced called kapp.k14s.io/renew-duration to set the duration at which user want to renew their resources.

Additional Notes for your reviewer:

Review Checklist:

Follows the developer guidelines
Relevant tests are added or updated
Relevant docs in this repo added or updated
Relevant carvel.dev docs added or updated in a separate PR and there's
a link to that PR
Code is at least as readable and maintainable as it was before this
change

Additional documentation e.g., Proposal, usage docs, etc.:

Proposal

praveenrewar

I will take another look at the implementation in some time.

pkg/kapp/clusterapply/add_or_update_change.go

pkg/kapp/cmd/app/deploy.go

test/e2e/priodic_update_test.go

pkg/kapp/cmd/app/deploy.go

pkg/kapp/config/default.go

pkg/kapp/diff/prepare_res_with_periodic_ann.go

pkg/kapp/clusterapply/add_or_update_change.go

pkg/kapp/diff/prepare_res_with_periodic_ann.go

pkg/kapp/diff/change_set_with_versioned_rs.go

test/e2e/priodic_update_test.go

pkg/kapp/diff/change_set_with_versioned_rs.go

pkg/kapp/config/default.go

pkg/kapp/diff/change_set_with_versioned_rs.go

100mik

A bunch of comments!
Main intention being to decouple renewed resource from versioned resources.
Still need to think about what we call things finally.

Kudos for the exhaustive tests, it handles every case I could think of! We should verify it does not add a flake because 2s might be less for slower hosts

pkg/kapp/diff/change_set_with_versioned_rs.go

100mik

How the feature is structured looks great to me!

@kumaritanushree mentioned that today if the user chooses to recreate on update, the last-renewed annotation is not retained. Maybe we should verify if the rebase rule is working.

I do not see a lot of harm in the timer being reset in this case (right away at least). But worth verifying. I am pointing towards the rebase rule as it looks like the feature works otherwise in other case as we check existing resources directly.

Lets also check the tests for flakiness since there's sleeping involved.

TL;DR Next steps:

Verify rebase rule
Ensure tests are not flaky (a few re runs on the GitHub action worker maybe?)

pkg/kapp/diff/change_set_with_versioned_rs.go

pkg/kapp/diff/changes_set_with_periodic_rs.go

pkg/kapp/cmd/app/deploy.go

pkg/kapp/diff/change_set_with_versioned_rs.go

pkg/kapp/diff/changes_set_with_periodic_rs.go

praveenrewar

Apart from the small nits, it looks good to me.

pkg/kapp/diff/renewable_resources.go

test/e2e/renewable-resources_test.go

…ff from clusterapply, added test case for the same

…ays-replace for non-versioned resources

pkg/kapp/diff/change_set_with_versioned_rs.go

pkg/kapp/diff/renewable_resources.go

…o improve functionality

100mik

LGTM!
I would prefer not having the unstopped functions, but dedicating a new structure to each one of them feels a bit extra

100mik · 2022-10-07T07:43:16Z

pkg/kapp/diff/change_set_with_versioned_rs.go

@@ -295,3 +275,40 @@ func existingVersionedResources(rs []ctlres.Resource) versionedResources {
 	}
 	return result
 }
+
+func newGroupedVersionedResources(rs []ctlres.Resource) map[string][]ctlres.Resource {


I think I would prefer it if this function and the next one are either scoped to a struct, maybe we can move stuff that are used by other parts of the code to a single place at a later point. Not a blocker but a nice to have

sethiyash

LGTM!

kumaritanushree requested a review from praveenrewar August 4, 2022 08:55

vmwclabot added the cla-not-required label Aug 4, 2022

praveenrewar reviewed Aug 4, 2022

View reviewed changes

sethiyash reviewed Aug 5, 2022

View reviewed changes

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

sethiyash reviewed Aug 5, 2022

View reviewed changes

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

praveenrewar reviewed Aug 5, 2022

View reviewed changes

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

pkg/kapp/cmd/app/deploy.go Outdated Show resolved Hide resolved

kumaritanushree requested review from rohitagg2020, renuy, praveenrewar, cppforlife, 100mik and sethiyash August 8, 2022 10:28

rohitagg2020 reviewed Aug 10, 2022

View reviewed changes

pkg/kapp/config/default.go Outdated Show resolved Hide resolved

pkg/kapp/diff/prepare_res_with_periodic_ann.go Outdated Show resolved Hide resolved

pkg/kapp/diff/prepare_res_with_periodic_ann.go Outdated Show resolved Hide resolved

100mik reviewed Aug 16, 2022

View reviewed changes

100mik reviewed Aug 17, 2022

View reviewed changes

pkg/kapp/diff/prepare_res_with_periodic_ann.go Outdated Show resolved Hide resolved

kumaritanushree marked this pull request as draft September 6, 2022 06:22

rohitagg2020 reviewed Sep 14, 2022

View reviewed changes

sethiyash reviewed Sep 14, 2022

View reviewed changes

test/e2e/priodic_update_test.go Outdated Show resolved Hide resolved

sethiyash reviewed Sep 15, 2022

View reviewed changes

pkg/kapp/diff/change_set_with_versioned_rs.go Outdated Show resolved Hide resolved

kumaritanushree marked this pull request as ready for review September 15, 2022 11:32

praveenrewar reviewed Sep 15, 2022

View reviewed changes

kumaritanushree force-pushed the task/224-priodic-resource-creation branch from deb8688 to e47b545 Compare September 15, 2022 14:59

rohitagg2020 reviewed Sep 16, 2022

View reviewed changes

pkg/kapp/config/default.go Outdated Show resolved Hide resolved

pkg/kapp/diff/change_set_with_versioned_rs.go Outdated Show resolved Hide resolved

kumaritanushree force-pushed the task/224-priodic-resource-creation branch 2 times, most recently from ec3a576 to c2e70d7 Compare September 16, 2022 12:54

rohitagg2020 reviewed Sep 19, 2022

View reviewed changes

pkg/kapp/diff/change_set_with_versioned_rs.go Outdated Show resolved Hide resolved

100mik reviewed Sep 21, 2022

View reviewed changes

kumaritanushree force-pushed the task/224-priodic-resource-creation branch from c2e70d7 to 9a3e42e Compare September 22, 2022 12:08

100mik reviewed Sep 22, 2022

View reviewed changes

pkg/kapp/diff/change_set_with_versioned_rs.go Outdated Show resolved Hide resolved

pkg/kapp/diff/changes_set_with_periodic_rs.go Outdated Show resolved Hide resolved

praveenrewar reviewed Sep 23, 2022

View reviewed changes

kumaritanushree force-pushed the task/224-priodic-resource-creation branch 2 times, most recently from 5a96c0e to e0a6ef1 Compare September 28, 2022 08:47

praveenrewar reviewed Sep 28, 2022

View reviewed changes

pkg/kapp/diff/renewable_resources.go Outdated Show resolved Hide resolved

test/e2e/renewable-resources_test.go Outdated Show resolved Hide resolved

Kumari Tanushree added 9 commits September 28, 2022 16:02

224: changes to update resources based on predetermined interval

4cf4a34

224: moved code changes for resource with periodic ann to diff pkg

3db0033

224: removed rebase rule and updated check for time expiry loop

5e49380

224: updated changes to move logic from deploy.go to diff package

eee7c04

224: updated e2e with new changes

2c17ec8

224: adopted comments

c56e5c8

224: moved check for update-strategy for non-versioned resource in di…

6bcef84

…ff from clusterapply, added test case for the same

224: updated changes to not force users to use update strategy as alw…

daf809b

…ays-replace for non-versioned resources

224: adopted comments, rebased with develop

d3c8702

kumaritanushree force-pushed the task/224-priodic-resource-creation branch 3 times, most recently from 4b0f079 to d656739 Compare October 3, 2022 14:08

praveenrewar reviewed Oct 4, 2022

View reviewed changes

pkg/kapp/diff/change_set_with_versioned_rs.go Outdated Show resolved Hide resolved

pkg/kapp/diff/renewable_resources.go Outdated Show resolved Hide resolved

224: adoped comments of renaming struct and apis, and updated logic t…

ed4b40f

…o improve functionality

kumaritanushree force-pushed the task/224-priodic-resource-creation branch from d656739 to ed4b40f Compare October 4, 2022 11:51

100mik approved these changes Oct 7, 2022

View reviewed changes

praveenrewar merged commit e14948a into develop Oct 7, 2022

sethiyash reviewed Oct 7, 2022

View reviewed changes

criztovyl mentioned this pull request Oct 16, 2022

Strip kustomize-style name hash suffix #517

Open

5 tasks

criztovyl mentioned this pull request Nov 6, 2022

diff internals: Introducing a DiffResource(s) struct #635

Open

praveenrewar deleted the task/224-priodic-resource-creation branch November 21, 2022 04:35

100mik mentioned this pull request Feb 3, 2023

Request: Role change for Approver carvel-dev/carvel#622

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes to update resources based on predetermined interval #571

changes to update resources based on predetermined interval #571

kumaritanushree commented Aug 4, 2022 •

edited

Loading

praveenrewar left a comment

100mik left a comment

100mik left a comment

praveenrewar left a comment

100mik left a comment

100mik Oct 7, 2022

sethiyash Oct 7, 2022

sethiyash left a comment

changes to update resources based on predetermined interval #571

changes to update resources based on predetermined interval #571

Conversation

kumaritanushree commented Aug 4, 2022 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Additional Notes for your reviewer:

Review Checklist:

Additional documentation e.g., Proposal, usage docs, etc.:

praveenrewar left a comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

praveenrewar left a comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

100mik Oct 7, 2022

Choose a reason for hiding this comment

sethiyash Oct 7, 2022

Choose a reason for hiding this comment

sethiyash left a comment

Choose a reason for hiding this comment

kumaritanushree commented Aug 4, 2022 •

edited

Loading