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

ecs: ability to access tag parameter value of TagParameterContainerImage #13202

Closed
1 of 2 tasks
Assignees
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@alastair-watts-avrios
Copy link
Contributor

alastair-watts-avrios commented Feb 22, 2021

It would be useful to be able to easily access the value of the docker image tag on a TagParameterContainerImage. Currently one can only access the parameter name which is very difficult to work with. The parameter itself can only be access by getting round the typing on TagParameterContainerImage and accessing the imageTagParameter field directly.

This change could give access to the whole parameter or just the value.

Use Case

Adding an environment variable with that version to the container should be easy and is very useful for having monitoring aware of what version is running. Basically useful whenever you want a CloudFormation to be able to see the version running.

Proposed Solution

add:

Other

add

  public get tagParameterValue(): string {
    return cdk.Lazy.string({
      produce: () => {
        if (this.imageTagParameter) {
          return this.imageTagParameter.valueAsString;
        } else {
          throw new Error('TagParameterContainerImage must be used in a container definition when using tagParameterName');
        }
      },
    });
  }

or

public get tagParameter(): string {
  return cdk.Lazy.string({
    produce: () => {
      if (this.imageTagParameter) {
        return this.imageTagParameter;
      } else {
        throw new Error('TagParameterContainerImage must be used in a container definition when using tagParameterName');
      }
    },
  });
}

to TagParameterContainerImage.

Happy to submit a PR with the first soon (as I think it fits with the existing method (and the move away from using parameters) best.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@alastair-watts-avrios alastair-watts-avrios added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Feb 22, 2021
@peterwoodworth peterwoodworth added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Feb 22, 2021
@skinny85 skinny85 changed the title ecs: ability to access tag parameter value ecs: ability to access tag parameter value of TagParameterContainerImage Mar 1, 2021
alastair-watts-avrios added a commit to alastair-watts-avrios/aws-cdk that referenced this issue Mar 2, 2021
…inerImage

Allows you to use the tag elsewhere within the container definition (e.g. to inform monitoring services of the release version).

fixes: aws#13202
@skinny85 skinny85 added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Mar 2, 2021
@skinny85
Copy link
Contributor

skinny85 commented Mar 2, 2021

Thanks for opening the issue @alastair-watts-avrios ! I agree this can be useful.

I'd love to have a PR for this 🙂. I like the first version a little more (it's more consistent with the existing tagParameterName method), so if you could go with that, it would be great 🙂.

Not sure if you've seen it already, but just to make sure, our Contributing guide is here: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md#linking-against-this-repository.

Thanks,
Adam

@alastair-watts-avrios
Copy link
Contributor Author

Thanks for having a look @skinny85.

Have submitted a PR (I went with the first method), #13340. Have tried to fallow the guidelines and the checks are passing so I hope it is ready to merge once it has approval.

@mergify mergify bot closed this as completed in #13340 Mar 8, 2021
mergify bot pushed a commit that referenced this issue Mar 8, 2021
…inerImage (#13340)

Allows you to use the tag elsewhere within the container definition (e.g. to inform monitoring services of the release version).

Fixes: #13202

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Mar 8, 2021

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment