Adding status reporting for RecoverableCondition #40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaypipes
reviewed
Apr 6, 2021
templates/pkg/resource/sdk.go.tpl
Outdated
Comment on lines
169
to
170
| // If terminal condition exists, other errors will | ||
| // will not be reported |
There was a problem hiding this comment.
This isn't actually true, though, right? If a NonTerminalCondition is found first in the list of Condition structs, we will still end up "reporting" that NonTerminalCondition...
There was a problem hiding this comment.
Yes, it was an oversight and I thought we were checking for the existing TerminalCondition below, but we are actually checking if the new error is Terminal. I fixed the comment to indicate that we continue to look for TerminalCondition even if we find a RecoverableError. TerminalCondition takes higher priority to display status as the sync stops.
There was a problem hiding this comment.
No worries, was just making sure I was reading the logic right :)
jkuruba
changed the title
surajkota
requested changes
Apr 7, 2021
surajkota
approved these changes
Apr 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: aws-controllers-k8s/community#498
Description of changes: RecoverableCondition is used to report errors that are not marked as Terminal. Currently controllers sync silently and errors are not reported. Reporting will notify Recoverable errors that can likely be fixed without re-applying the spec.
Testing: Ran reconciler unit tests
go test -v
=== RUN TestReconcilerUpdate
2021-04-05T18:51:38.576Z INFO ackrt updated resource {"kind": "Book", "namespace": "default", "name": "mybook", "generation": 1, "is_adopted": false}
--- PASS: TestReconcilerUpdate (0.00s)
=== RUN TestRegistry
--- PASS: TestRegistry (0.00s)
=== RUN TestServiceController
--- PASS: TestServiceController (0.00s)
=== RUN TestIsAdopted
--- PASS: TestIsAdopted (0.00s)
=== RUN TestIsSynced
--- PASS: TestIsSynced (0.00s)
PASS
ok github.com/aws-controllers-k8s/runtime/pkg/runtime 0.016s
Also, tested if the errors are reported in SageMaker manually for AccessDeniedException.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.