Skip to content
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

Load service model name from generator #216

Merged
merged 4 commits into from
Oct 7, 2021

Conversation

RedbackThomson
Copy link
Contributor

Fixes aws-controllers-k8s/community#994

Description of changes:
This pull request supports including an optional model_name field in the generator.yaml file. The generator will use this argument to override the service name when looking up the API files in aws-sdk-go/models/apis.

Currently ServiceIDClean is used in all code generator templates as the import path for aws-sdk-go and when referencing the controller name (eg. {{ .ServiceIDClean}}-controller). This pull request will remove ServiceIDClean and instead use the service alias (as ServiceAlias) when referencing the controller and the aws-sdk-go packages.

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

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teeny suggestion if you re-spin, but I'm good with this as-is. Thank you @RedbackThomson!

@a-hilaly @brycahta since Vijay is out today and tomorrow, please review and lgtm if all good with this.

@@ -207,3 +212,67 @@ func getSDKVersionFromGoMod(goModPath string) (string, error) {
}
return "", fmt.Errorf("couldn't find %s in the go.mod require block", sdkModule)
}

// loadModelWithLatestAPIVersion finds the AWS SDK for a given service alias and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the code below, considering (in a future PR if you agree with me) perhaps putting this into an importable package under pkg/

pkg/generate/config/config.go Outdated Show resolved Hide resolved
cmd/ack-generate/command/common.go Outdated Show resolved Hide resolved
}

// loadModel finds the AWS SDK for a given service alias and creates a new model
// with the given API version.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if apiVersion is empty string?

m, err := ackmodel.New(
sdkAPI, "", optGeneratorConfigPath, ackgenerate.DefaultConfig,
)
m, err := loadModel(svcAlias, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you add a comment as to why the apiVersion is empty here? If it doesn't matter, then loadModelWithLatestAPIVersion would be clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually couldn't tell you why apiVersion needs to be empty string, only for release. I get an abstract error when I provide the latest API version. I believe this may because of the multiversion support - you would not need to specify an API version since it assumes each of them individually to create the webhook.

Copy link
Contributor Author

@RedbackThomson RedbackThomson Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@a-hilaly I would love your input on this.

pkg/generate/config/config.go Show resolved Hide resolved
pkg/model/sdk_helper.go Outdated Show resolved Hide resolved
pkg/generate/config/config.go Show resolved Hide resolved
Copy link
Contributor

@brycahta brycahta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@brycahta
Copy link
Contributor

brycahta commented Oct 7, 2021

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Oct 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, 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:
  • OWNERS [RedbackThomson,jaypipes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 79b8dd7 into aws-controllers-k8s:main Oct 7, 2021
@RedbackThomson RedbackThomson deleted the model-in-generator branch October 7, 2021 21:05
acornett21 pushed a commit to acornett21/ack-code-generator that referenced this pull request Oct 12, 2021
Fixes aws-controllers-k8s/community#994

Description of changes:
This pull request supports including an optional `model_name` field in the `generator.yaml` file. The generator will use this argument to override the service name when looking up the API files in aws-sdk-go/models/apis.

Currently `ServiceIDClean` is used in all code generator templates as the import path for aws-sdk-go and when referencing the controller name (eg. `{{ .ServiceIDClean}}-controller`). This pull request will remove `ServiceIDClean` and instead use the service alias (as `ServiceAlias`) when referencing the controller and the aws-sdk-go packages. 

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-sdk-go package name doesn't match service ID
4 participants