Use loadModel instead of loadModelWithLatestAPIVersion for crossplane #240
Conversation
getLatestAPIVersion doesn't seem to work correctly with crossplance, and opgGenVersion is already used to find the configuration path for resources, so just use that instead.
|
Hi @EdgeJ. Thanks for your PR. I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
ack-bot
added
the
needs-ok-to-test
|
/ok-to-test |
ack-bot
added
ok-to-test
|
@EdgeJ - Thanks for the quick fix. Glad you were on top of it. I'm still getting loadModel is using the default ackgenerate.DefaultConfig and not the one created for Crossplane in pkg/generate/crossplane/ See for reference.It looks like this was changed in 79b8dd7#diff-6c9b11a8ab6c3f28f4ad6ee4d3f7f5b1c9da81f13ac772b15c784aa95f0ded10 and we haven't caught it until now. loadModel needs to support passing in alternative sdkHelpers and defaultConfigs, or else we will need to duplicate the flow for the crossplane resources. |
|
/hold |
ack-bot
added
the
do-not-merge/hold
Thanks for the insight @AaronME , I didn't realize the commit adding the model name configuration wiped out some necessary configs for crossplane. I don't think it should be too difficult to add that back into with this PR, but as you pointed out, we may just need to duplicate some of the functionality in |
Ugh I totally missed that detail in that PR. I will make a note to include a crossplane smoke test in our e2e test suite so that this doesn't happen again. Appreciate you taking the time to diagnose this! |
|
/unhold |
ack-bot
removed
the
do-not-merge/hold
|
Added some flags to the root command to be able to control what default configs to use (right now, only the ACK default and Crossplane default) and to dynamically set the API group name. I think this should take care of the issues we were seeing. At any rate, tests out well on my machine doing dry run builds of some existing resources. |
I don't expect that anyone would ever use the ACK defaults with this crossplane generator - so I am happy with just hardcoding that and removing the option. As for the API group name, is this something crossplane would likely override? |
I don't think it's too likely. I'll go ahead and remove those flags and just hard code the crossplane command to set the specific configurations in loadModel instead. |
EdgeJ
force-pushed
the
fix-crossplane-api-versions
branch
from
d3a6c7b
to
4605874
Compare
|
/retest |
|
|
||
| if cpAPIGroupSuffix != "" { | ||
| sdkAPI.APIGroupSuffix = cpAPIGroupSuffix | ||
| } |
There was a problem hiding this comment.
Instead of the conditionals above, how about adding new apiGroup and defaultCfg parameters to the loadModel function?
There was a problem hiding this comment.
Yeah, this is the best approach. I think I didn't consider this before because I was trying to touch as few places as possible, but it turns out only one other package uses this function outside the ones I've already changed.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EdgeJ, jaypipes, RedbackThomson 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 |
getLatestAPIVersion doesn't seem to work correctly with crossplane, and
opgGenVersion is already used to find the configuration path for
resources, so just use that instead.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.