-
Notifications
You must be signed in to change notification settings - Fork 226
Initial implementation of AWSResourceManager.LateInitialize() method. #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
IMO, this PR should be good to review, merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niice! Great start on this @vijtrip2!
I left few comments below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this implementation could be significantly simpler... with virtually no code-generating changes needed at all.
…late initialization.
…queueDelay method.
…e.yaml for ecr test-data.
Issue: aws-controllers-k8s/community#812 Changes: * Introduce new condition ConditionTypeLateInitialized * Update reconciler to invoke `AWSResourceManager.LateInitialize()` method ------------------ Corresponding Code generator PR: aws-controllers-k8s/code-generator#138 Please do not merge until code generator PR is merged ------------------ Testing * Unit tests successful ``` ➜ runtime git:(li-mlp) ✗ make test building mocks for pkg/types ... ok. building mocks for k8s.io/apimachinery/pkg/apis/meta/v1 ... ok. building mocks for k8s.io/apimachinery/runtime ... ok. building mocks for k8s.io/apimachinery/runtime/schema ... ok. building mocks for sigs.k8s.io/controller-runtime/pkg/client ... ok. go test ./... ? github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1 [no test files] ? github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/apis/meta/v1 [no test files] ? github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime [no test files] ? github.com/aws-controllers-k8s/runtime/mocks/apimachinery/pkg/runtime/schema [no test files] ? github.com/aws-controllers-k8s/runtime/mocks/controller-runtime/pkg/client [no test files] ? github.com/aws-controllers-k8s/runtime/mocks/pkg/types [no test files] ok github.com/aws-controllers-k8s/runtime/pkg/compare (cached) ok github.com/aws-controllers-k8s/runtime/pkg/condition (cached) ? github.com/aws-controllers-k8s/runtime/pkg/config [no test files] ? github.com/aws-controllers-k8s/runtime/pkg/errors [no test files] ? github.com/aws-controllers-k8s/runtime/pkg/metrics [no test files] ok github.com/aws-controllers-k8s/runtime/pkg/requeue (cached) ok github.com/aws-controllers-k8s/runtime/pkg/runtime 1.287s ok github.com/aws-controllers-k8s/runtime/pkg/runtime/cache (cached) ? github.com/aws-controllers-k8s/runtime/pkg/runtime/log [no test files] ? github.com/aws-controllers-k8s/runtime/pkg/types [no test files] ok github.com/aws-controllers-k8s/runtime/pkg/util (cached) ? github.com/aws-controllers-k8s/runtime/pkg/webhook [no test files] ``` * local-kind-e2e-test logs ``` │ │ 2021-07-26T19:38:08.701Z DEBUG ackrt > r.Sync {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "kind": "Repository" │ │ 2021-07-26T19:38:08.701Z DEBUG ackrt >> rm.ReadOne {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": f │ │ 2021-07-26T19:38:08.701Z DEBUG ackrt >>> rm.sdkFind {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": │ │ 2021-07-26T19:38:09.641Z DEBUG ackrt <<< rm.sdkFind {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": │ │ 2021-07-26T19:38:09.641Z DEBUG ackrt << rm.ReadOne {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": f │ │ 2021-07-26T19:38:09.641Z DEBUG ackrt >> r.setResourceManaged {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_a │ │ 2021-07-26T19:38:09.641Z DEBUG ackrt >>> kc.Patch (metadata + spec) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2" │ │ 2021-07-26T19:38:09.694Z DEBUG ackrt <<< kc.Patch (metadata + spec) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2" │ │ 2021-07-26T19:38:09.694Z DEBUG ackrt marked resource as managed {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "i │ │ 2021-07-26T19:38:09.694Z DEBUG ackrt << r.setResourceManaged {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_a │ │ 2021-07-26T19:38:09.694Z DEBUG ackrt >> rm.Create {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": fa │ │ 2021-07-26T19:38:09.694Z DEBUG ackrt >>> rm.sdkCreate {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted" │ │ 2021-07-26T19:38:09.725Z DEBUG ackrt <<< rm.sdkCreate {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted" │ │ 2021-07-26T19:38:09.725Z DEBUG ackrt << rm.Create {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": fa │ │ 2021-07-26T19:38:09.725Z INFO ackrt created new resource {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopt │ │ 2021-07-26T19:38:09.725Z DEBUG ackrt >> rm.LateInitialize {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adop │ │ 2021-07-26T19:38:09.725Z INFO ackrt calculated late initialization delay is 0 seconds {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "re │ │ 2021-07-26T19:38:09.725Z DEBUG ackrt >>> rm.sdkFind {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": │ │ 2021-07-26T19:38:09.748Z DEBUG ackrt <<< rm.sdkFind {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": │ │ 2021-07-26T19:38:09.748Z DEBUG ackrt << rm.LateInitialize {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adop │ │ 2021-07-26T19:38:09.748Z DEBUG ackrt >> r.patchResourceMetadataAndSpec {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west │ │ 2021-07-26T19:38:09.748Z DEBUG ackrt >>> kc.Patch (metadata + spec) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2" │ │ 2021-07-26T19:38:09.758Z DEBUG ackrt <<< kc.Patch (metadata + spec) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2" │ │ 2021-07-26T19:38:09.759Z DEBUG ackrt patched resource metadata and spec {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-wes │ │ 2021-07-26T19:38:09.759Z DEBUG ackrt << r.patchResourceMetadataAndSpec {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west │ │ 2021-07-26T19:38:09.759Z DEBUG ackrt >> r.patchResourceStatus {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_ │ │ 2021-07-26T19:38:09.759Z DEBUG ackrt >>> kc.Patch (status) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_ado │ │ 2021-07-26T19:38:09.794Z DEBUG ackrt <<< kc.Patch (status) {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_ado │ │ 2021-07-26T19:38:09.795Z DEBUG ackrt patched resource status {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_a │ │ 2021-07-26T19:38:09.795Z DEBUG ackrt << r.patchResourceStatus {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_ │ │ 2021-07-26T19:38:09.795Z DEBUG ackrt < r.Sync {"kind": "Repository", "namespace": "default", "name": "ecr-repository-wy6n6wjm8", "generation": 1, "account": "309117047740", "role": "", "region": "us-west-2", "is_adopted": false, │ │ 2021-07-26T19:38:09.795Z DEBUG controller-runtime.controller Successfully Reconciled {"controller": "repository", "request": "default/ecr-repository-wy6n6wjm8"} ```
@vijtrip2 OK, I just cut the v0.10.0 release of ACK runtime with the related LateInitialize changes. Please update this PR to bring in a new ACK runtime go.mod dependency, update the |
@jaypipes , Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vijtrip2 this is very close. I have one requested change to make in this PR (see inline, it's about returning just latest, err
instead of a requeue needed when we error in the ReadOne call...)
pkg/testdata/models/apis/ecr/0000-00-00/generator-with-late-initialize.yaml
Show resolved
Hide resolved
Done.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm OK with this functionally, thank you @vijtrip2 :)
In a future PR, please wrap code and code comments at 80 chars to follow the existing style as much as possible (80 chars makes demo and screen-sharing of the code easier since it allows the code to fit in a single screen without horizontal scrolling, FWIW).
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, vijtrip2 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 |
Issue #, if available: aws-controllers-k8s/community#812
Description of changes:
AWSResourceManager.LateInitialize()
method code generatorlate_initialize_pre_read_one
&late_initialize_post_read_one
Testing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.