Add Archive Resource
#11
Conversation
|
@a-hilaly ready for your E2E tests IMHO |
a-hilaly
force-pushed
the
archive
branch
4 times, most recently
from
7fb9e50 to
d8e2e8a
Compare
|
/lgtm |
|
@embano1: you cannot LGTM your own PR. In response to this:
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. |
RedbackThomson
left a comment
RedbackThomson
left a comment
There was a problem hiding this comment.
Some comments below - especially want to point out we can save a hook for setting synced=true based on status
| if !archiveAvailable(&resource{ko}) { | ||
| ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil) | ||
| } else { | ||
| ackcondition.SetSynced(&resource{ko}, corev1.ConditionTrue, nil, nil) | ||
| } |
There was a problem hiding this comment.
We already have this type of functionality built into the generator - to use statuses as the indicator for synced. Here is an example of how we do it for the apigatewayv2-controller: https://github.com/aws-controllers-k8s/apigatewayv2-controller/blob/main/generator.yaml#L98-L102
There was a problem hiding this comment.
Gotcha. So in all cases where I do not need to pass a message to SetSynced, e.g. ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil) I will remove this line and let code-gen figure out.
cc/ @a-hilaly should also work in such cases when I need to set other conditions then?
if archiveInTerminalState(latest) {
msg := "Archive is in '" + *latest.ko.Status.State + "' status"
ackcondition.SetTerminal(desired, corev1.ConditionTrue, &msg, nil)
ackcondition.SetSynced(desired, corev1.ConditionTrue, nil, nil) // TODO: remove
return desired, nil
}Using this generator.yaml syntax:
synced:
when:
- path: Status.State
in:
- ENABLED
- DISABLED
- CREATE_FAILED
- UPDATE_FAILEDThere was a problem hiding this comment.
Hm, not sure this one will work then?
if archiveCreating(latest) {
msg := "Archive is currently being created"
ackcondition.SetSynced(desired, corev1.ConditionFalse, &msg, nil)
return desired, requeueWaitUntilCanModify(latest)
}Would code-gen SYNCED path respect my message here?
There was a problem hiding this comment.
I get the feeling to keep the logic as is for now as this change would require more testing (incl. E2Es) now to verify all the possible combinations again.
There was a problem hiding this comment.
I've noticed that when you use is_synced with other hooks, other components of the reconciliation logic can break. I've seen it happen before with lambda.
So, to keep things simple and speed up development for this controller, I'm cool with merging the current code as is and dealing with the issue in a separate PR for all the CRDs. That way, if the problem pops up with this controller, we'll know how to tackle it without getting bogged down in everything else.
| @@ -0,0 +1,19 @@ | |||
| if archiveInTerminalState(latest) { | |||
| msg := "Archive is in '" + *latest.ko.Status.State + "' status" | |||
There was a problem hiding this comment.
nit: Move these messages into constants in hook.go and use fmt.Sprintf
There was a problem hiding this comment.
Will change. Was following other controllers where this is a pattern and thought this is to avoid fmt allocations :D
There was a problem hiding this comment.
Changed the wording using fmt.Sprintf but kept it in the same location because there was no more readable way IMHO to express these messages by moving to separate file.
Signed-off-by: Amine Hilaly <hilalyamine@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, embano1, jaypipes 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:
Approvers can indicate their approval by writing |
4 participants
feat: Add
ArchiveresourceBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.