Apply as many changes as possible with error summary at end #532

praveenrewar · 2022-06-18T10:41:41Z

What this PR does / why we need it:

Currently kapp errors out as soon as it detects an error while apply a resource or waiting for a resource, but we would want to apply as many changes as possible and return all the errors at end. If a change doesn't depend on a failed change then it should get applied.

Changes that depend on failing changes shouldn't be applied
exit-early-on-apply-error flag can be used to exit as soon as an error is encountered while applying changes (default true)
exit-early-on-wait-error flag can be used to exit as soon as an error is encountered while waiting for changes (default true)

Sample gist for testing: https://gist.github.com/praveenrewar/2746318c5a273d13ed26497f34755638

TODO:

Add e2e test
Do more manual testing

Which issue(s) this PR fixes:

Fixes #426

Does this PR introduce a user-facing change?

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.:

praveenrewar · 2022-06-19T06:21:26Z

pkg/kapp/clusterapply/cluster_change_set.go

 		if err != nil {
 			return err
 		}


Should the timeout errors also include the unsuccessfulChanges?

I think they should!

I am still thinking if we should do this and what would be the best way to present the errors.

100mik

Some thoughts:

Should there be a flag which enables disables this behaviour? What should be it's default value?
How does this look with multiple failures? I think we might need better formatting than [error1, error2, error3] maybe more like:

- error1
- error2
- error3

praveenrewar · 2022-06-21T20:24:16Z

Should there be a flag which enables disables this behaviour? What should be it's default value?

Hmm, maybe, but I am not sure why someone would want to turn it off.

How does this look with multiple failures?

It's just like you have mentioned 😁 please do take a look at the gist that I have shared, it contains an example manifest and the output that we would get while deploying it.

100mik · 2022-06-22T04:38:21Z

Hmm, maybe, but I am not sure why someone would want to turn it off.

One case I can think of is pipelines. They would probably want to error out the moment something goes south rather than wait for other resources to be created.
Some packages might be interested in this I guess, but I do not see a super strong case for it. (Not sure if kapp-controller will choose to error out by default because the installation fails none the less)

It's just like you have mentioned 😁 please do take a look at the gist that I have shared, it contains an example manifest and the output that we would get while deploying it.

Oooh noice! Missed that. I noticed there was a ']' added to a test and misunderstood 🤔

praveenrewar · 2022-06-22T07:23:19Z

They would probably want to error out the moment something goes south rather than wait for other resources to be created.

I don't know how that would be useful, but maybe it would be useful in some scenarios that I am not able to think at the moment, so I am keeping this discussion open for now, we can discuss more later.

I noticed there was a ']' added to a test and misunderstood

Yeah, I am still trying to figure out how to best use NewSemiStructuredError. Right now it formats properly when there are more errors (>1), but if there is just one error then it keeps the brackets. I will look more into it.

pkg/kapp/clusterapply/applying_changes.go

test/e2e/custom_wait_rules_test.go

cppforlife · 2022-07-05T18:30:51Z

we should probably have a flag that fails on first failure (current behaviour) so that folks can still "quickly" exit if that's what they want.
how does this feature play with timeout errors? should we be erroring on timeout errors immediately?

praveenrewar · 2022-07-05T18:42:10Z

we should probably have a flag that fails on first failure (current behaviour) so that folks can still "quickly" exit if that's what they want.

Yeah, that's something @100mik had suggested as well. I had kept this discussion open till now, I will add a flag to disable the behaviour.

how does this feature play with timeout errors?

Right now, we error out for --wait-timeout but not for --wait-resource-timeout.

should we be erroring on timeout errors immediately?

I was thinking that individual resource timeouts can be treated as resource errors in this case, as one resource getting timed out doesn’t mean that others (which don't depend on it) will also get timed out.

test/e2e/formatted_error_test.go

praveenrewar · 2022-07-25T06:50:07Z

pkg/kapp/clusterapply/cluster_change_set.go

 		waitingChanges.Track(appliedChanges)

 		if waitingChanges.IsEmpty() {
+			if len(unsuccessfulChanges) == 1 {


NewSemiStructuredError doesn't work really well when there is only one error inside [] because there might be errors containing something like map[string]string and there is no good way to differentiate between an error and such field.
I should probably add this as a comment.

100mik · 2022-07-25T21:40:53Z

The changes themselves LGTM 🤔

Re: Error structuring
I think we are:

Retaining how we used to show errors for one error by using the len(unsuccessfulChanges) == 1 check
However multiple errors won't have the formatting due to capitalisation?

kumaritanushree · 2022-07-26T11:48:54Z

Suggestion: I think it would be good if you can add little more description like: how the new output will look like? You can add some example output.

praveenrewar · 2022-07-26T11:52:56Z

Retaining how we used to show errors for one error by using the len(unsuccessfulChanges) == 1 check

That's correct.

However multiple errors won't have the formatting due to capitalisation?

That doesn't happen in NewSemistructuredError.

praveenrewar · 2022-07-26T11:54:12Z

Suggestion: I think it would be good if you can add little more description like: how the new output will look like? You can add some example output.

I think the test cases and the gist that I have provided should be a good indication of the output, but do let me know if I should add more examples.

100mik · 2022-07-26T11:57:25Z

So I am guessing multiple errors will be unformatted 🤔
Also, I m not entirely sure if the way we format a single message will help in case of multiple messages.
But I think we should explore if we have any options better than having it in one line.

That said I do not think it is a major blocker but more of a nice to have thing imo 🚀

sethiyash

LGTM. Just thinking about the output. For the example given in the gist wouldn't it be better to show 2 Resource Succeded before showing errors or final summary at the end on number of resources succeeded and failed.

praveenrewar · 2022-07-27T17:29:29Z

wouldn't it be better to show 2 Resource Succeded before showing errors or final summary at the end on number of resources succeeded and failed.

Hmm, definitely a good thought. The reason I didn't include more details around successful changes at the end is because we already include enough information in the logs (ok: reconcile...). I also was trying to keep the ui changes to the minimum, but I am open to creating a separate PR if we want to enhance the experience a bit further later on.

100mik · 2022-08-01T08:50:28Z

I think I am satisfied with how the UI changes look as well, I cannot think of how we can make a list of errors look better right away. But these cosmetic improvements can always go as a separate PR if we have better ideas.

100mik · 2022-08-01T08:50:59Z

Also, @praveenrewar looks like this branch needs a rebase. I will take one final look at this once it is done!

100mik

A few nitty thoughts!
Over all the implementation looks good 🚀

pkg/kapp/cmd/app/apply_flags.go

pkg/kapp/clusterapply/applying_changes.go

pkg/kapp/cmd/app/apply_flags.go

- Changes that depend on failing changes shouldn't be applied - exit-on-apply-error flag can be used to exit as soon as an error is encountered while applying changes - exit-on-wait-error flag can be used to exit as soon as an error is encountered while waiting for changes (default true)

100mik

LGTM!

vmwclabot added the cla-not-required label Jun 18, 2022

praveenrewar marked this pull request as draft June 18, 2022 10:41

praveenrewar commented Jun 19, 2022

View reviewed changes

100mik reviewed Jun 21, 2022

View reviewed changes

rohitagg2020 reviewed Jun 23, 2022

View reviewed changes

pkg/kapp/clusterapply/applying_changes.go Show resolved Hide resolved

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

praveenrewar force-pushed the error-at-end branch 2 times, most recently from 3f6c521 to 7cfe3b9 Compare July 3, 2022 19:17

praveenrewar marked this pull request as ready for review July 4, 2022 04:57

praveenrewar force-pushed the error-at-end branch 2 times, most recently from 05a67c8 to ee45a27 Compare July 6, 2022 13:43

cppforlife reviewed Jul 19, 2022

View reviewed changes

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

praveenrewar force-pushed the error-at-end branch 2 times, most recently from 47192dc to c5b2777 Compare July 21, 2022 14:31

praveenrewar commented Jul 25, 2022

View reviewed changes

praveenrewar force-pushed the error-at-end branch from c5b2777 to aeadf6f Compare July 26, 2022 17:14

sethiyash reviewed Jul 27, 2022

View reviewed changes

praveenrewar force-pushed the error-at-end branch from aeadf6f to b2deed6 Compare August 1, 2022 09:16

100mik reviewed Aug 1, 2022

View reviewed changes

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

pkg/kapp/clusterapply/applying_changes.go Show resolved Hide resolved

praveenrewar force-pushed the error-at-end branch 2 times, most recently from a4389a9 to 937b6fa Compare September 8, 2022 06:13

rohitagg2020 reviewed Sep 12, 2022

View reviewed changes

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

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

praveenrewar force-pushed the error-at-end branch from 937b6fa to 082da4c Compare September 13, 2022 05:16

praveenrewar force-pushed the error-at-end branch from 082da4c to 98398e3 Compare September 15, 2022 08:44

100mik approved these changes Sep 15, 2022

View reviewed changes

praveenrewar merged commit 094843d into develop Sep 19, 2022

praveenrewar deleted the error-at-end branch September 21, 2022 05:31

praveenrewar mentioned this pull request Oct 28, 2022

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

Merged

19 tasks

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

Apply as many changes as possible with error summary at end #532

Apply as many changes as possible with error summary at end #532

praveenrewar commented Jun 18, 2022 •

edited

Loading

praveenrewar Jun 19, 2022

100mik Jun 21, 2022

praveenrewar Jul 4, 2022 •

edited

Loading

100mik left a comment

praveenrewar commented Jun 21, 2022 •

edited

Loading

100mik commented Jun 22, 2022 •

edited

Loading

praveenrewar commented Jun 22, 2022

cppforlife commented Jul 5, 2022 •

edited

Loading

praveenrewar commented Jul 5, 2022

praveenrewar Jul 25, 2022 •

edited

Loading

100mik commented Jul 25, 2022

kumaritanushree commented Jul 26, 2022

praveenrewar commented Jul 26, 2022

praveenrewar commented Jul 26, 2022

100mik commented Jul 26, 2022

sethiyash left a comment •

edited

Loading

praveenrewar commented Jul 27, 2022

100mik commented Aug 1, 2022

100mik commented Aug 1, 2022

100mik left a comment

100mik left a comment

Apply as many changes as possible with error summary at end #532

Apply as many changes as possible with error summary at end #532

Conversation

praveenrewar commented Jun 18, 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 Jun 19, 2022

Choose a reason for hiding this comment

100mik Jun 21, 2022

Choose a reason for hiding this comment

praveenrewar Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

praveenrewar commented Jun 21, 2022 • edited Loading

100mik commented Jun 22, 2022 • edited Loading

praveenrewar commented Jun 22, 2022

cppforlife commented Jul 5, 2022 • edited Loading

praveenrewar commented Jul 5, 2022

praveenrewar Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

100mik commented Jul 25, 2022

kumaritanushree commented Jul 26, 2022

praveenrewar commented Jul 26, 2022

praveenrewar commented Jul 26, 2022

100mik commented Jul 26, 2022

sethiyash left a comment • edited Loading

Choose a reason for hiding this comment

praveenrewar commented Jul 27, 2022

100mik commented Aug 1, 2022

100mik commented Aug 1, 2022

100mik left a comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

praveenrewar commented Jun 18, 2022 •

edited

Loading

praveenrewar Jul 4, 2022 •

edited

Loading

praveenrewar commented Jun 21, 2022 •

edited

Loading

100mik commented Jun 22, 2022 •

edited

Loading

cppforlife commented Jul 5, 2022 •

edited

Loading

praveenrewar Jul 25, 2022 •

edited

Loading

sethiyash left a comment •

edited

Loading