feat(aws-ecs): expose environment from containerDefinition

[biffgaut]

Closes #17867

• Assigned props.environment to a public readonly member
• Added integration test that confirms the environment can be appended after the task is instantiated

Made 2 cosmetic, but no obvious changes. Environment values are specified:

name: value
name2: value

But in the test and the README.md files the sample values were:

name: something
value: something else

This is using the string 'value" as a key - which, as someone reading the code for the first time, was confusing. So I changed the sample values to more clearly display what's a key and what's a value.

---

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

[gitpod-io]

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[clarencetw]

Great update. LGTM.

[rix0rrr]

If the whole point of exposing this member field publicly is, so that it can be mutated: sorry, no dice.

First of all, this will not work across jsii boundaries (as dictionaries are serialized by value and not by reference there), and reaching into the state of another class is not typically a recommended design choice. It happens to work for this case (by accident), but a strategy like this will not work for all cases. For example, it will not work if the ContainerDefinition was directly represented as a resource in CloudFormation (try it on another class!).

The thing is: even though we don't have the mechanisms in place to enforce this, you are really really encouraged to treat those public members as readonly and immutable.

The better way to expose this feature is by providing an addEnvironment(key, value) method, like a Lambda Function does: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html#addwbrenvironmentkey-value-options

[rix0rrr]

Also.

You originally changed:

{
      name: 'I_WAS_TRIGGERED',
      value: 'From CloudWatch Events'
}

Into

{
      name: 'value',
      source: 'From CloudWatch Events'
}

Which I found confusing (name: 'value' threw me off here). I thought the reasoning behind this was that environment was a { name -> value } mapping, instead of a list of { name, value } records.

So in turn, I changed it to:

{
  I_WAS_TRIGGERED: 'From CloudWatch Events'
}

In order to preserve the original semantics of setting a single environment variable with a single value.

However, now that I changed it, I'm getting the following build error:

�[96m@aws-cdk.aws-ecs-README-L631.ts�[0m:�[93m52�[0m:�[93m7�[0m - �[91merror�[0m�[90m TS2322: �[0mType '{ I_WAS_TRIGGERED: string; }' is not assignable to type 'TaskEnvironmentVariable'.
  Object literal may only specify known properties, and 'I_WAS_TRIGGERED' does not exist in type 'TaskEnvironmentVariable'.

�[7m52�[0m       I_WAS_TRIGGERED: 'From CloudWatch Events'
�[7m  �[0m �[91m      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~�[0m
[jsii-rosetta] [WARN] 1 diagnostics from assemblies with 'strict' mode on (and 1 more non-strict diagnostics)

And it turns out that indeed, its not a map of { name -> value }, it is a list of records (which I should have seen, because the type IS a list):

export interface ContainerOverride {
  /**
   * Variables to set in the container's environment
   */
  readonly environment?: TaskEnvironmentVariable[];
}

/**
 * An environment variable to be set in the container run as a task
 */
export interface TaskEnvironmentVariable {
  /**
   * Name for the environment variable
   *
   * Exactly one of `name` and `namePath` must be specified.
   */
  readonly name: string;

  /**
   * Value of the environment variable
   *
   * Exactly one of `value` and `valuePath` must be specified.
   */
  readonly value: string;
}

https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-events-targets/lib/ecs-task-properties.ts/#L17

So I don't know why you didn't get an error for the original change (you should have), but the original code was correct, and all of our modifications are incorrect.

[biffgaut]

Thanks - nice object lesson for everyone to wait for AWS to comment on the original issue before writing code :-).

I will update (rewrite) and resubmit based on a simplified version of how Lambda handles the issue (Lambda has more complexity due to deployments at edge and extra CDK created environment variables).

Pull request has been modified.

[biffgaut]

I will leave the doc and example as they are as requested, but I think your example from ContainerOverride doesn't represent ContainerDefintionProps. Here's ContainerOverride:

export interface ContainerOverride {
  /**
   * Variables to set in the container's environment
   */
  readonly environment?: TaskEnvironmentVariable[];
}

Where environment is a list of { name: something, value: something } objects. But environment in ContainerDefinitionProps is not a list, it specifies each environment variable as a a new attribute:

readonly environment?: { [key: string]: string };

My intention (unless you request differently beforehand or upon review) is to have the signature of addEnvironmentVariables follow this paradigm. I'm not a fan of using attributes to represent a list (I like []), but it seems like addEnvironmentVariables should be consistent with how they are specified in props.

(I also need to dive into ContainerOverride, I'm not familiar with that and don't know if that should figure into my implementation)

Edited - I see what you mean now, the doc I changed in README was for ContainerOverride, not ContainerDefintion.

[rix0rrr]

Edited - I see what you mean now, the doc I changed in README was for ContainerOverride, not ContainerDefintion.

Yes exactly.

The reason it's different for Events's RunEcsTask is because both the environment variable name, as well as the value, could be taken from the event message, and there's no other way to represent that (iirc)

Pull request has been modified.

[biffgaut]

FWIW - rixOrrr approved this, but before it merge it drifted from Master and refreshing it instigated the need for another approval (no change was made on my end besides the merge). I'm a first time contributor, so the workflows are not enabled yet.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 2345d91
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).