Skip to content

Conversation

@RedbackThomson
Copy link
Contributor

@RedbackThomson RedbackThomson commented Oct 4, 2021

Issue #, if available: aws-controllers-k8s/community#994

Description of changes:
This pull request replaces all templates and variables with a defined nomenclature. The current code-generator uses service name terms interchangeably, which eventually lead to the linked issue. While PR #211 solved the logical issue of overriding the service model name, in some cases, it did not do a clear job of clarifying what names exist for any service and when they are applicable.

There are 3 different names that are used to programmatically reference a service. Here are the terms I am using in this PR to refer to them, and their given purpose:

  • Service ID
    • The metadata.serviceID field from the model api-2.json file
  • Service model name
    • The path to the model api-2.json file
  • Service package name
    • The AWS SDK Go package path
    • Used by the ACK controller's for their name

Some examples on when these differ:

  • Step Functions uses sfn for its ID and alias, but states for its model name
  • Elastic Load Balancing v2 uses elbv2 for its alias, but elasticloadbalancingv2 for its ID and model name

There are also 2 names defined in the controller metadata file that are not used in any parts of the code, but are used in documentation:

  • Service name
    • The full name of the service as defined by the product's documentation page (eg. Amazon Elastic Cloud Compute)
  • Service short name
  • An informal, shortened name of the service (eg. EC2)

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

Copy link
Contributor

@vijtrip2 vijtrip2 left a comment

Choose a reason for hiding this comment

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

LGTM! Very nice work!

@vijtrip2
Copy link
Contributor

vijtrip2 commented Oct 5, 2021

/test all

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.

I actually think #211 should be reverted.

If there are services like opensearchservice and elbv2 that have a name for the model package in aws-sdk-go/models/apis directory, we should add a field to the generator.yaml file that allows the code generator to understand this. Asking developers to both remember to supply a --model-name argument and remember what that argument value should be is a bad contributor experience, particularly for the team that is maintaining those peculiar services.

@RedbackThomson
Copy link
Contributor Author

I actually think #211 should be reverted.

If there are services like opensearchservice and elbv2 that have a name for the model package in aws-sdk-go/models/apis directory, we should add a field to the generator.yaml file that allows the code generator to understand this. Asking developers to both remember to supply a --model-name argument and remember what that argument value should be is a bad contributor experience, particularly for the team that is maintaining those peculiar services.

Yeah in retrospect this is a way better solution. I absolutely agree. I have created #215 to revert this.

@RedbackThomson
Copy link
Contributor Author

Check out #216 for loading the service alias from the generator. I will rename ServiceAlias to ServicePackage in this PR (#213) once it is merged. Might be a bit of a rebase hell.

@RedbackThomson
Copy link
Contributor Author

None of the e2e tests will pass, because none of them have been updated to runtime v0.15.0. I don't understand what is happening in those tests for them to even compile.

@jaypipes
Copy link
Collaborator

jaypipes commented Oct 8, 2021

None of the e2e tests will pass, because none of them have been updated to runtime v0.15.0. I don't understand what is happening in those tests for them to even compile.

It's not that the tests are not compiling the controllers properly. They are.

It's that the tests are starting the controller using a now-deprecated flag:

unknown flag: --aws-account-id

jaypipes added a commit to jaypipes/ack-code-generator that referenced this pull request Oct 8, 2021
ACK runtime v0.15.0 removed support for the `--aws-account-id`
controller flag but our deployment templates were still adding this flag
to the controller entrypoint args, which causes tests to fail with:

```
unknown flag: --aws-account-id
```

Related: aws-controllers-k8s#213

See: aws-controllers-k8s/runtime@6080101

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
ack-bot pushed a commit that referenced this pull request Oct 8, 2021
ACK runtime v0.15.0 removed support for the `--aws-account-id`
controller flag but our deployment templates were still adding this flag
to the controller entrypoint args, which causes tests to fail with:

```
unknown flag: --aws-account-id
```

Related: #213

See: aws-controllers-k8s/runtime@6080101

Signed-off-by: Jay Pipes <jaypipes@gmail.com>

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@RedbackThomson
Copy link
Contributor Author

/test dynamodb-controller-test
/test ecr-controller-test
/test s3-controller-test

acornett21 pushed a commit to acornett21/ack-code-generator that referenced this pull request Oct 12, 2021
ACK runtime v0.15.0 removed support for the `--aws-account-id`
controller flag but our deployment templates were still adding this flag
to the controller entrypoint args, which causes tests to fail with:

```
unknown flag: --aws-account-id
```

Related: aws-controllers-k8s#213

See: aws-controllers-k8s/runtime@6080101

Signed-off-by: Jay Pipes <jaypipes@gmail.com>

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@RedbackThomson
Copy link
Contributor Author

@vijtrip2 @jaypipes Any more to add here? Happy with this?

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.

Much cleaner. ++

@jaypipes
Copy link
Collaborator

/lgtm

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

ack-bot commented Oct 13, 2021

[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:
  • OWNERS [RedbackThomson,jaypipes,vijtrip2]

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

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.

4 participants