Allow manual setting of AWS client struct and interface names #422

a-hilaly · 2023-03-03T00:40:49Z

While developing the ACK pipes-controller, we encountered an issue where
the generated code didn't compile mainly because the code-generator
assumed that the AWS SDK client/interface/import-paths always followed
the same naming conventions. However, we discovered that Pipes was the
exception as they renamed the model, import path, and client
struct/interface.

This patch introduces a new feature that enables users to manually set
the client struct and interface names in the generator.yaml file.
This update allows for greater flexibility in the AWS client naming
within pipes-controller and possibly other controllers in the future.

In this patch, we've also introduced a breaking change by renaming
model_name to sdk_names.model. Mainly to keep all the SDK-related
naming conventions in the same place.

e.g:

old configuration:

model_name: foo

new configuration:

sdk_names:
  model_name: foo

Signed-off-by: Amine Hilaly [email protected]

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

a-hilaly · 2023-03-03T04:00:00Z

/test lambda-controller-test

jaypipes · 2023-03-03T13:47:05Z

Pipes are always a problem.

jaypipes

@a-hilaly @embano1 I think it would be better to just rely on the aws-sdk-go/private/model/api.API InterfaceName and StructName methods.

a-hilaly · 2023-03-03T17:24:21Z

@a-hilaly @embano1 I think it would be better to just rely on the aws-sdk-go/private/model/api.API InterfaceName and StructName methods.

@jaypipes Unfortunately, those don't work properly for us. The reason is that the aws-sdk-go/pipes package was generated using flags different than the default ones. And since we don't have access to those there is nothing we can do (i think)

jaypipes · 2023-03-06T14:56:59Z

@a-hilaly @embano1 I think it would be better to just rely on the aws-sdk-go/private/model/api.API InterfaceName and StructName methods.

@jaypipes Unfortunately, those don't work properly for us. The reason is that the aws-sdk-go/pipes package was generated using flags different than the default ones. And since we don't have access to those there is nothing we can do (i think)

@a-hilaly can you prove this? I don't understand why those two methods won't work for this use case...

a-hilaly · 2023-03-06T15:12:47Z

@jaypipes api.InterfacePackageName returns eventbridgepipesiface (we need pipesiface) and api.StructName returns EventBridgePipes (we need Pipes/PipesAPI). Note that the values returned by &api{} are correct theoritically speaking, but pipes teams generated their controller using different struct/interface/package names: probably with something like

go run -tags codegen ./private/model/cli/gen-api/main.go -use-service-id -path pipes ./models/apis/pipes/2015-10-07/api-2.json

jaypipes · 2023-03-06T15:55:12Z

pkg/model/sdk_api.go

+	if a == nil || a.API == nil {
+		return ""
+	}
+	return fmt.Sprintf("%s%s", a.API.StructName(), "API")


Should this be a.API.InterfaceName() by default?

There is no a.API.InterfaceName :/ . However there a a.API.InterfacePackageName but it return the package name that defines the interface.. which is not what we need here.

Previously we used to do something similar within the Go templates: https://github.com/aws-controllers-k8s/code-generator/pull/422/files#diff-427fe62c26e82668e3c6509c81e7230831d3e652b8e836cc7a54be8a52c84405L68

Yeah, looking in the upstream code they indeed use StructName ... :/

While developing the ACK pipes-controller, we encountered an issue where the generated code didn't compile mainly because the code-generator assumed that the AWS SDK client/interface/import-paths always followed the same naming convetions. However, we discovered that pipes was the exception as they renamed the model, import path and client struct/interface. This patch introduces a new feature that enables users to manually set the client structg and interface names int he `generator.yaml` file. This update allows for greater flexibvility in the AWS client naming within pipes-controller and possibly other controllers in the future. In this patch, we've also introduced a breaking change by renaming `model_name` to `sdk_names.model`. Mianly to keep all the SDK related naming convetions in the same place. Signed-off-by: Amine Hilaly <[email protected]>

ack-prow · 2023-03-10T16:05:08Z

@a-hilaly: The following test 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	`e371c49`	link	true	`/test ec2-controller-test`

Full PR test history. Your PR dashboard.

Details

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

Happy to approve this. I'm a bit tentative about the naming, only because I don't fully understand the difference between these. Would you mind adding examples for each of these to the PR description for our reference? (examples of the normal default, and then what pipes does differently?)

jaypipes

👍

jaypipes · 2023-03-14T12:48:42Z

pkg/model/sdk_api.go

+	if a == nil || a.API == nil {
+		return ""
+	}
+	return fmt.Sprintf("%s%s", a.API.StructName(), "API")


Yeah, looking in the upstream code they indeed use StructName ... :/

jaypipes · 2023-03-14T12:49:06Z

/lgtm

ack-prow · 2023-03-14T12:49:12Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,RedbackThomson,jaypipes]

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

bobh66 · 2023-03-21T03:40:45Z

@jaypipes After this commit I'm seeing errors when running ack-generate crossplane for the prometheusservice in the crossplane-contrib/provider-aws project:

./ack-generate crossplane --aws-sdk-go-version v1.44.174 prometheusservice --output ../../
Error: service prometheusservice not found
+ exit 1

The Makefile is generating a list of services from the directories in https://github.com/crossplane-contrib/provider-aws/tree/master/apis that contain generator_config.yaml files. This logic worked until these changes were applied.

I can dig into it more tomorrow but I thought I would check to see if it's something obvious that we can change on the provider-aws side.

Thanks

haarchri · 2023-03-21T05:37:58Z

@bobh66 you need to switch

model_name to sdk_names.model

https://github.com/crossplane-contrib/provider-aws/blob/master/apis/prometheusservice/generator-config.yaml#L1

bobh66 · 2023-03-21T14:53:21Z

Thanks @haarchri - that fixed it.

For future reference if this comes up as a search result, the specific change is:

model_name: foo

to

sdk_names:
  model_name: foo

a-hilaly · 2023-03-21T15:06:18Z

modified PR description to include the example you provided @bobh66 . Thank you folks!

ack-prow bot requested review from RedbackThomson and vijtrip2 March 3, 2023 00:40

ack-prow bot added the approved label Mar 3, 2023

jaypipes requested changes Mar 3, 2023

View reviewed changes

jaypipes reviewed Mar 6, 2023

View reviewed changes

a-hilaly force-pushed the sdk-names branch from 123e349 to 8d86a78 Compare March 10, 2023 13:22

a-hilaly force-pushed the sdk-names branch from 8d86a78 to e371c49 Compare March 10, 2023 16:00

RedbackThomson approved these changes Mar 13, 2023

View reviewed changes

jaypipes approved these changes Mar 14, 2023

View reviewed changes

ack-prow bot assigned jaypipes Mar 14, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2023

jaypipes merged commit 5404da8 into aws-controllers-k8s:main Mar 14, 2023

Allow manual setting of AWS client struct and interface names #422

Allow manual setting of AWS client struct and interface names #422

Uh oh!

Conversation

a-hilaly commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly commented Mar 3, 2023

Uh oh!

jaypipes commented Mar 3, 2023

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypipes commented Mar 6, 2023

Uh oh!

a-hilaly commented Mar 6, 2023

Uh oh!

jaypipes Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

a-hilaly Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Mar 10, 2023

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

jaypipes Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

jaypipes commented Mar 14, 2023

Uh oh!

ack-prow bot commented Mar 14, 2023

Uh oh!

bobh66 commented Mar 21, 2023

Uh oh!

haarchri commented Mar 21, 2023

Uh oh!

bobh66 commented Mar 21, 2023

Uh oh!

a-hilaly commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

a-hilaly commented Mar 3, 2023 •

edited

Loading

a-hilaly commented Mar 3, 2023 •

edited

Loading