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

feat(aws-ecs): Adds an addSecret() method for a ContainerDefinition #18960

Closed
wants to merge 14 commits into from
Closed

feat(aws-ecs): Adds an addSecret() method for a ContainerDefinition #18960

wants to merge 14 commits into from

Conversation

danwiltshire
Copy link
Contributor

I'm using Aspects to configure already-instantiated resources in CDK. This PR will enable adding a Secret to a ContainerDefinition, similar to addEnvironment().

Had to remove the readonly modifier to allow the addSecret() method to set or push secrets. Let me know if there's a better way of handling this.

Added an integration test, worked as expected in my environment.

closes #18959


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

@gitpod-io
Copy link

gitpod-io bot commented Feb 13, 2022

@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Feb 13, 2022
@robertd
Copy link
Contributor

robertd commented Feb 22, 2022

@madeline-k Can you please review this and merge in when time permits? We need this in our current ECS projects & workflows. TIA.

Copy link
Contributor

@madeline-k madeline-k left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this feature, @danwiltshire! Nice work. I just have one comment.

packages/@aws-cdk/aws-ecs/lib/container-definition.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed madeline-k’s stale review February 24, 2022 20:21

Pull request has been modified.

@danwiltshire
Copy link
Contributor Author

@madeline-k Hello, I updated the PR as requested but these tests are now failing. Do you know how to resolve this?

Thanks

API elements with incompatible changes:
err  - PROP @aws-cdk/aws-ecs.ContainerDefinition.referencesSecretJsonField: changed from property to method [changed-kind:@aws-cdk/aws-ecs.ContainerDefinition.referencesSecretJsonField]
err  - PROP @aws-cdk/aws-ecs.FirelensLogRouter.referencesSecretJsonField: changed from property to method [changed-kind:@aws-cdk/aws-ecs.FirelensLogRouter.referencesSecretJsonField]

@rix0rrr rix0rrr added feature-request A feature should be added or improved. p1 and removed p1 feature-request A feature should be added or improved. @aws-cdk/aws-ecs Related to Amazon Elastic Container labels Mar 4, 2022
@aws-cdk-automation
Copy link
Collaborator

This PR has been in BUILD FAILING for 21 days, and looks abandoned. It will be closed in 10 days if no further commits are pushed to it.

@anelson
Copy link

anelson commented May 3, 2022

@madeline-k it looks like this PR has gone stale. We really need this ability in our project. Is there anything I could do to get this moving forward?

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 199bd32
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

} else {
return false;
}
}
Copy link

@jishi jishi May 17, 2022

Choose a reason for hiding this comment

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

Maybe use getter syntax to allow it to be accessed as a property is what needs to be changed:

public get referencesSecretJsonField(): boolean {

That would allow it to be accessed as obj.referencesSecretJsonField thus not breaking API spec.

Copy link

Choose a reason for hiding this comment

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

Additionally, it could probably also just check length of secrets array thus not requiring the private backing field.

@github-actions
Copy link

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@github-actions
Copy link

github-actions bot commented Jun 2, 2022

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@github-actions github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 2, 2022
@github-actions github-actions bot closed this Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-ecs): addSecret() for a ContainerDefinition
7 participants