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-cdk): add support for user provided repository name to DockerImageAsset #2032

Merged
merged 17 commits into from
Mar 19, 2019

Conversation

alex-berger
Copy link
Contributor

@alex-berger alex-berger commented Mar 18, 2019

Add support for (optional) user provided repository name to DockerImageAsset. This change makes it easier to use (reference) Docker Images created and deployed by CDK from EKS (Kubernetes) YAML resource files. In contrast to ECS/Fargate Tasks one cannot directly pass dynamically created ECR repository names to Kubernetes Pod resource definitions (YAML files) as these resources are not managed by CloudFormation. This PR allows users to use predefined (well-known) ECR repository names, which can then be refered to from within Kubernetes Pod resource definitions using the latest image tag. For example the Docker Image created by:

new DockerImageAsset(this, 'MyImage, {
   directory: path.join(__dirname + "images"  , 'my-image'),
   repositoryName: "my/ecr/repository"
});

Can then be referenced from a Kubernetes Pod resource definition like this:

spec:
  containers:
  - name: MyContainer
    image: 000000000000.dkr.ecr.eu-central-1.amazonaws.com/my/ecr/repository:latest

Note, this changes do not affect the synthesized CloudFormation template as the repository is passed
into the stack as parameter. Futhermore, there are not unit tests as I was not able to figure out how to
add unit test for ToolkitInfo. I tested this new feature manually and it worked well, pusing the resutling Docker Image into an ECR repository with the expected repository name.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

…geAsset. This change makes it easier to use (reference) Docker Images created and deployed by CDK from EKS (Kubernetes) YAML resource files.
@alex-berger alex-berger requested a review from a team as a code owner March 18, 2019 09:00
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Thanks for the submission. I will try and add a test after merging some other PRs.

@rix0rrr rix0rrr self-assigned this Mar 18, 2019
[email protected] and others added 15 commits March 18, 2019 13:11
…ctions (aws#2020)

Additional input Artifacts are needed when using the `parameterOverrides` property of the Action (any Artifact used in that map needs to be added to the input Artifacts of the Action, otherwise the Pipeline will fail at runtime).

Also did some cleanup in the Action superclass while I was in the area.

Fixes aws#1247
Update class name in docs, remove outdated paragraph.
Without this fix, the `Not` condition would render as (notice that `variable`, `comparisonOperator` and `value` are not capitalized):
```
{ Not:
   VariableComparison { variable: '$.a', comparisonOperator: 0, value: 'b' } }
```
Update the build tools to take an argument to pack only a subset of language targets.
If only stack Outputs are changed, CloudFormation generates a ChangeSet
that is executable but has 0 changes.

Before, we looked at the amount of changes to say there was nothing to
do, but now we look at the actual change set status to determine
whether it's an empty change set or not.

The effect is that we can now deploy updates even if only Outputs
changed. This becomes very important when the only thing changed
to a stack is an Output got added because a cross-stack reference
was taken by a downstream stack.

Fixes aws#778.
Because of accepted Tokenized values, cfn2ts previously translated a
string and string array into:

    interface Props {
        someString: string;
        stringArray: Array<string | Token> | Token;
    }

The latter was not intended. We now emit:

    interface Props {
        someString: string;
        stringArray: string[];
    }

Since the Token will be typed as `Object` in Java, this change now makes
it impossible to mistakenly pass any old java Object. A typical case
would be pass a `Subnet` where a `string subnetId` was expected, failing
in a nonobvious way.

Fixes aws#2030.
The role input parameters are currently Roles but should be IRoles.

Required adding the grant methods to the `IRole` definition, which
weren't there before.

Fixes aws#1925.
Also in this commit:

- Bugfix to the update script to compile, so create-new-libraries is
  actually run on the new spec.
- Add 'npm run set-refkind' to make it easier to set refkinds for
  new resources.
…tion") (aws#2047)

The method `toCloudFormation` is not supposed to be directly called by
users. Mark it as `@internal` and rename to `_toCloudFormation`.

Fixes aws#2044
Related aws#2016

BREAKING CHANGE: “toCloudFormation” is now internal and should not be called directly. Instead use “app.synthesizeStack”
Update `instanceCount` => `desiredCapacity` in ECS README.

Fixes aws#1926.
@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 19, 2019

Alex, I cannot push updates to this branch, probably because it's based off of your master branch. Is there a checkbox to the right on the PR to give me push permissions, as seen here?

https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork

If not, can you please make a branch and update this PR to track your branch instead?

@alex-berger
Copy link
Contributor Author

alex-berger commented Mar 19, 2019

Yes Allow edits from maintainers is enabled on this PR, so according the Github documentation you should be able to push to my master branch. Let me know if it still does not work and I will replace this PR with a new one from another (non-master) branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants