-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add addoption by annotation feature #164
Conversation
These changes introduce a new feature gate called `ForceAdoptResources` which allows users to provide the read required fields in the annotation and an empty spec, and the controller would populate the resource and adopt from AWS
003da8c
to
3fd1d42
Compare
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.
Great work on this Mr Michael! i left a few comments below
// | ||
// NOTE(michaelhtm): Done, tnx :) |
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.
🚀
This change will allow us to support an `adopt-or-create` policy in the future, where the controller will create the resource when the resource does not exist and can't be adopted. Before we support `adopt-or-create` we need a solution in code-gen where we change how we handle the `PopulateResourceFromAnnotation` which currently only allows the population of fields that are required for a readOne operation, and they happen to be all scalar fields (besides ARN, but we have a way of handling that), but the required fields for create would need to be sometimes structs, and this would require users to provide values in form of maps eg. Creating an EKS cluster requires a ResourceVPCConfig, which is a struct that contains subnetIDs etc. We can have a couple of ways to address this. 1. Accept these values in the spec, and return terminal error when we attempt a create and the create required fields are not provided 2. Accept these values in the `adoption-fields` annotation. This would need a code-gen change to allow reading from structs and assigning fields. but it would also make the annotation easy to make mistakes with when using yaml
c783c67
to
4d78380
Compare
@michaelhtm: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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.
Great work on this Michael! merging to see how tests are doing on code-gen
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm 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:
Description of changes:
These changes introduce a new feature gate called
ResourceAdoption
which allows users to provide the
Read
required fields in the annotation andan empty spec, and the controller would populate the resource and adopt
from AWS
Currently we are considering two values for the
adoption-policy
annotation,one being
adopt
, which will just try to adopt a resource, and if not found,keep trying until a resource to adopt exists, or
adopt-or-create
which willbe able to create a resource if adoption fails because resource doesn't exist.
Before we support
adopt-or-create
we need a solution in code-genwhere we change how we handle the
PopulateResourceFromAnnotation
which currently only allows the population of fields that are
required for a readOne operation, and they happen to be all scalar
fields (besides ARN, but we have a way of handling that), but the
required fields for create would need to be sometimes structs,
and this would require users to provide values in form of maps
eg.
Creating an EKS cluster requires a ResourceVPCConfig, which is a
struct that contains subnetIDs etc.
We can have a couple of ways to address this.
we attempt a create and the create required fields are not provided
adoption-fields
annotation. This wouldneed a code-gen change to allow reading from structs and assigning
fields. but it would also make the annotation easy to make mistakes
with when using yaml
Here's an example for cluster:
Here's another one to adopt a nodegroup
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.