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

remove --aws-account-id flag from deployment #218

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

jaypipes
Copy link
Collaborator

@jaypipes jaypipes commented 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.

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>
@jaypipes
Copy link
Collaborator Author

jaypipes commented Oct 8, 2021

@RedbackThomson in order to get past the test failures, this PR will likely need to be manually merged and then a new release of code-generator cut.

@brycahta
Copy link
Contributor

brycahta commented Oct 8, 2021

@RedbackThomson in order to get past the test failures, this PR will likely need to be manually merged and then a new release of code-generator cut.

The test runner, kind-build-test.sh is also using the flag hence the failures.

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

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

ack-bot commented Oct 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, jaypipes

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-bot ack-bot merged commit 7c52167 into aws-controllers-k8s:main Oct 8, 2021
@surajkota
Copy link
Member

@brycahta
Copy link
Contributor

brycahta commented Oct 8, 2021

@jaypipes @brycahta need to remove from helm charts https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.yaml.tpl#L41

Functionally , this is fine today right? I agree it should be cleaned up along with its use here: test-helm.sh

@jaypipes
Copy link
Collaborator Author

jaypipes commented Oct 8, 2021

@jaypipes @brycahta need to remove from helm charts https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.yaml.tpl#L41

No, the environment variables are fine. It's just the command-line flag that was at issue.

@RedbackThomson
Copy link
Contributor

@jaypipes @brycahta need to remove from helm charts https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.yaml.tpl#L41

No, the environment variables are fine. It's just the command-line flag that was at issue.

We still should remove the Helm value and corresponding environment variable from the templates - they serve no purpose anymore

@jaypipes
Copy link
Collaborator Author

jaypipes commented Oct 8, 2021

@jaypipes @brycahta need to remove from helm charts https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.yaml.tpl#L41

No, the environment variables are fine. It's just the command-line flag that was at issue.

We still should remove the Helm value and corresponding environment variable from the templates - they serve no purpose anymore

They actually do serve a purpose. They allow overriding the GetCallerIdentity results.

@RedbackThomson
Copy link
Contributor

I don't believe setting the environment variable has any effect in the AWS SDK Go - https://github.com/aws/aws-sdk-go/search?q=AWS_ACCOUNT_ID ? Through what mechanism would it override those results?

@jaypipes
Copy link
Collaborator Author

jaypipes commented Oct 8, 2021

I don't believe setting the environment variable has any effect in the AWS SDK Go - https://github.com/aws/aws-sdk-go/search?q=AWS_ACCOUNT_ID ? Through what mechanism would it override those results?

Sorry, was confusing with AWS_ACCESS_KEY_ID: https://github.com/aws/aws-sdk-go/blob/main/aws/credentials/env_provider.go

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.
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.

5 participants