feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

Dzhuneyt · 2022-07-07T18:21:12Z

Closes #10952

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

…ion with `userDataCausesReplacement` (#18726) If both `addSignalOnExitCommand` _and_ `userDataCausesReplacement` are used it results in an invalid logicalId being used in the `cfn-signal` call. This is due to `addSignalOnExitCommand` getting the logicalID from `Stack.getLogicalId` which does not take into consideration logicalId overrides which `userDataCausesReplacement` uses. This updates `addSignalOnExitCommand` to use the `logicalId` of the resource which is evaluated lazily and happens after all overrides. fixes #12749 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

gitpod-io · 2022-07-07T18:21:19Z

TheRealAmazonKendra · 2022-07-12T23:03:36Z

Thank you for your contribution! I'll wait on the build succeeding to do a thorough review here, but just a few quick comments while you're still making updates:

Please see our contributing guide for formatting the title of PRs and for the information required in the body of the PR.
It looks like you've debated about whether this is a chore, feat, or fix. I'd classify this as a feat.
Some of the formatting updates you've done are great but most aren't necessary and make finding/reviewing the actual change much harder. Please either make those formatting changes as a separate PR or minimize them so we can focus on the change in functionality.
I think there's a typo in the readme update. It doesn't quite make sense to me.

TheRealAmazonKendra

Looks like other services integration tests are impacted by these changes. Please update those as well. Running a full build locally will help you identify each of those instances. It takes longer, but it will mean less churn.

Pull request has been modified.

Dzhuneyt · 2022-07-18T22:51:45Z

Thank you for all the patience @TheRealAmazonKendra! This is my first time doing a CDK contribution that involves writing integration tests, so I need some time to learn the ropes.

Hopefully, I can wrap up this PR in a day or two.

TheRealAmazonKendra · 2022-07-19T00:18:37Z

Thank you for all the patience @TheRealAmazonKendra! This is my first time doing a CDK contribution that involves writing integration tests, so I need some time to learn the ropes.

Hopefully, I can wrap up this PR in a day or two.

We are more than happy both to have you as a contributor and to answer questions while you're figuring stuff out! If you have any questions about why certain tests are failing, feel free to ping me and I'll be more than happy to help troubleshoot. In this case, I see that it's because the expected values are hardcoded instead of being a reference to region and account. I'm pretty sure updating lines 11 and 15 in BatchDefaultEnvVarsStack.template.json and lines 50 and 54 in tree.json will get that test passing. I'm not sure if tests further down will then fail, though.

Dzhuneyt · 2022-07-20T08:15:48Z

@TheRealAmazonKendra the build seems to succeed now. Again, thank you for being so patient on this. :)

TheRealAmazonKendra · 2022-07-20T17:59:19Z

packages/@aws-cdk/aws-batch/lib/job-definition.ts

    return {
      command: container.command,
-      environment: this.deserializeEnvVariables(container.environment),
+      environment,


So, I'm wondering if always having values here is a breaking change for customers currently using this. Breaking changes are OK in an experimental module, but I'm wondering if we should make setting these defaults an option in the props instead of just doing it 100% of the time. I'm also wondering if there are use cases where a user would want this undefined. I'm not super familiar with AWS Batch so I think I need to consult with others on this.

I'm not 100% sure about this specific scenario, but generally speaking, from my experience - when we talk about breaking change in the code of some library - it's usually the removal of parameters, functions, classes, or a modification of their existing behavior, whereas additions of fields, parameters, methods are not considered a breaking change.

But again, that's just my perspective, looking at other libraries I've consumed in my projects as a developer. With CDK it's slightly different since we are considering not just the code but also things that users might have deployed using that code, and I realize it's a slightly more flavored and opinionated discussion.

TheRealAmazonKendra

After talking with the Batch team, I don't see any scenarios where this would cause an issue. Approving.

mergify · 2022-07-21T22:28:06Z

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

aws-cdk-automation · 2022-07-21T23:04:11Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: be06bff
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-07-21T23:04:22Z

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

…r, if they are not explicitly set (aws#21041) Closes aws#10952 ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Dzhuneyt and others added 9 commits January 31, 2022 14:49

Merge remote-tracking branch 'remote/master'

fb6de3d

Merge branch 'aws:master' into master

6a35cbd

Merge branch 'aws:master' into master

f2958c6

Merge branch 'aws:master' into master

5531ac1

Merge branch 'aws:master' into master

846ecd3

Merge branch 'aws:master' into master

ee14ff1

Merge branch 'aws:main' into master

2720ad1

feat: Pass default AWS_REGION and AWS_ACCOUNT

7fef2a2

github-actions bot added the p2 label Jul 7, 2022

aws-cdk-automation requested a review from a team July 7, 2022 18:21

Dzhuneyt changed the title ~~Dzhuneyt/issue 10952 aws batch env name~~ feat: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set Jul 7, 2022

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Jul 7, 2022

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

e726152

Dzhuneyt changed the title ~~feat: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set~~ chore: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set Jul 7, 2022

Dzhuneyt added 3 commits July 7, 2022 22:03

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

4fbcfcb

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

cb8e781

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

cf07efd

Dzhuneyt changed the title ~~chore: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set~~ feat: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set Jul 12, 2022

Dzhuneyt and others added 5 commits July 12, 2022 15:05

chore: Update README.md

248319c

feat: Pass default environment variables

364c74a

feat: Pass default environment variables

d488b86

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

c8bd318

chore: Fix tests

52a9b4c

Dzhuneyt changed the title ~~feat: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set~~ fix: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set Jul 12, 2022

Dzhuneyt changed the title ~~fix: Add fallback AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set~~ feat: add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set Jul 13, 2022

chore: Restore original formatting

66d92aa

Dzhuneyt and others added 6 commits July 14, 2022 15:54

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

9dd3c6b

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

552e393

chore: Update integration test snapshots

aa4478e

chore: Update integration test snapshots

c2f1ac2

chore: Update integration test snapshots

66dc657

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

6968942

TheRealAmazonKendra previously requested changes Jul 18, 2022

View reviewed changes

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

420eba8

Dzhuneyt and others added 3 commits July 19, 2022 01:31

chore: Update integration test snapshots

85fd70a

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

d3d3323

chore: Update integration test snapshot of deps

752defa

Dzhuneyt and others added 4 commits July 19, 2022 09:39

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

d828c92

chore: Update integration test snapshots

963aa6e

chore: Update integration test snapshots

2a025dd

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

2366c3f

TheRealAmazonKendra reviewed Jul 20, 2022

View reviewed changes

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

0eab1f6

TheRealAmazonKendra approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name

be06bff

mergify bot merged commit eed854e into aws:main Jul 21, 2022

Dzhuneyt deleted the dzhuneyt/issue-10952-aws-batch-env-name branch July 22, 2022 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

Dzhuneyt commented Jul 7, 2022 •

edited

Loading

gitpod-io bot commented Jul 7, 2022

TheRealAmazonKendra commented Jul 12, 2022

TheRealAmazonKendra left a comment

Dzhuneyt commented Jul 18, 2022

TheRealAmazonKendra commented Jul 19, 2022

Dzhuneyt commented Jul 20, 2022

TheRealAmazonKendra Jul 20, 2022

Dzhuneyt Jul 21, 2022 •

edited

Loading

TheRealAmazonKendra left a comment

mergify bot commented Jul 21, 2022

aws-cdk-automation commented Jul 21, 2022

mergify bot commented Jul 21, 2022

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

Conversation

Dzhuneyt commented Jul 7, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 7, 2022

TheRealAmazonKendra commented Jul 12, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Dzhuneyt commented Jul 18, 2022

TheRealAmazonKendra commented Jul 19, 2022

Dzhuneyt commented Jul 20, 2022

TheRealAmazonKendra Jul 20, 2022

Choose a reason for hiding this comment

Dzhuneyt Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 21, 2022

aws-cdk-automation commented Jul 21, 2022

AWS CodeBuild CI Report

mergify bot commented Jul 21, 2022

Dzhuneyt commented Jul 7, 2022 •

edited

Loading

Dzhuneyt Jul 21, 2022 •

edited

Loading