Skip to content

Remove optGenVersion from api path for goimports #430

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

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

bobh66
Copy link
Contributor

@bobh66 bobh66 commented Mar 20, 2023

Description of changes:

ack-generate crossplane fails if there is no apis/_service_/v1alpha1 directory. The code includes optGenVersion in the apiPath for goimports which causes an error when the directory doesn't exist.

Removing optGenVersion from the apiPath causes goimports to run on all files/directorys under apis/<service>

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-generate crossplane fails if there is no apis/<service>/v1alpha1 directory.
The code includes optGenVersion in the api path for goimports which
causes an error when the directory doesn't exist.

Removing optGenVersion from the api path causes goimports to run on all
files/directorys under apis/<service>

Signed-off-by: Bob Haddleton <[email protected]>
@ack-prow ack-prow bot requested review from a-hilaly and vijtrip2 March 20, 2023 20:45
@ack-prow ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 20, 2023
@ack-prow
Copy link

ack-prow bot commented Mar 20, 2023

Hi @bobh66. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@haarchri
Copy link
Contributor

@a-hilaly can you have a look ? unblocks us for an PR crossplane-contrib/provider-aws#1704

@a-hilaly
Copy link
Member

@bobh66 @haarchri how about just passing --version=""? Nothing breaks with the previously generated CRDs?

/ok-to-test

@ack-prow ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 20, 2023
@a-hilaly
Copy link
Member

ec2-controller failures are unrelated. Please ignore :)

@bobh66
Copy link
Contributor Author

bobh66 commented Mar 20, 2023

@a-hilaly the crossplane sub-command does not accept the --version flag so it fails with:

Error: unknown flag: --version

I would not expect any impact on previously generated CRDs as this is only affecting the execution of goimports to validate the generated API and controller files.

@a-hilaly
Copy link
Member

a-hilaly commented Mar 20, 2023

@bobh66 Gotcha, thank you! merging this soon.
/lgtm

@ack-prow ack-prow bot added lgtm Indicates that a PR is ready to be merged. approved labels Mar 20, 2023
@a-hilaly a-hilaly merged commit 2c79531 into aws-controllers-k8s:main Mar 20, 2023
@ack-prow
Copy link

ack-prow bot commented Mar 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, bobh66

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow
Copy link

ack-prow bot commented Mar 20, 2023

@bobh66: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ec2-controller-test 63eac8f link true /test ec2-controller-test
dynamodb-controller-test 63eac8f link true /test dynamodb-controller-test

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.

RedbackThomson pushed a commit to RedbackThomson/ack-code-generator that referenced this pull request Mar 21, 2023
…#430)

`ack-generate crossplane` fails if there is no `apis/_service_/v1alpha1 directory`. The code includes optGenVersion in the apiPath for goimports which causes an error when the directory doesn't exist.

Removing optGenVersion from the apiPath causes `goimports` to run on all files/directorys under `apis/<service>`

Signed-off-by: Bob Haddleton <[email protected]>

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants