proposal for supporting resource references #1057
Conversation
vijtrip2
force-pushed
the
res-rel-proposal
branch
from
fab4955 to
e8e4255
Compare
There was a problem hiding this comment.
I'm super happy with the overall formula for this proposal. It's a shame we have to create a separate reference field, rather than overriding the existing ones, but I understand the limitation.
I have some concerns regarding the format of the generator - I would like to reduce complexity for the service teams as much as possible and I think we can simplify it greatly. See the inline comments for more details.
vijtrip2
force-pushed
the
res-rel-proposal
branch
2 times, most recently
from
2222e6f to
ef755f9
Compare
| gt := "*ackv1alpha1.AWSResourceReference" | ||
| gtp := "*ackv1alpha1.AWSResourceReference" |
There was a problem hiding this comment.
We may want to make this configurable, since Crossplane has its own referencer type:
/cc @muvaf
There was a problem hiding this comment.
@jaypipes , can you please elaborate on this?
I am assuming that you are pointing towards taking the reference type as input(option/env variable) from user who performs make build-controller? The default will be ackv1alpha1.AWSResourceReference .
This will allow crossplane to generate CRDs compatible with their reference type.
There was a problem hiding this comment.
I believe he's referring to the fact that crossplane has a separate set of config defaults that are used exclusively when running the ackgenerate crossplane verb - https://github.com/aws-controllers-k8s/code-generator/blob/main/pkg/generate/crossplane/config.go#L19
|
@jaypipes: GitHub didn't allow me to request PR reviews from the following users: muvaf. Note that only aws-controllers-k8s members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
vijtrip2
force-pushed
the
res-rel-proposal
branch
from
ef755f9 to
a576653
Compare
|
@jaypipes , do you think this is good enough to be merged now? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
Will this support getting fields from a service in the future? i.e getting a loadbalancer dns name to be used in an intergration as a url? |
If you mean will the controller make AWS ELB |
|
Not quite, we currently use the k8s service to create load balancers and it would be nice if we could reference that service in an intergration to get the dns name. Other wise it’s a copy and paste job |
|
@Leweyy, Can you please create a GH issue for the same so that in future ACK team can circle back on implementing such functionality. |
Issue #545
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.