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(ecr): add imageTagMutability prop #10557

Merged
merged 11 commits into from
Mar 9, 2021
Merged

Conversation

ap00rv
Copy link
Contributor

@ap00rv ap00rv commented Sep 26, 2020

This property allows setting tag mutability on ECR repositoes. Tag mutability is useful to ensure image integrity and can prevent supply chain attacks.

Closes #4640


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

This property allows setting tag mutability on ECR repositoes. Tag mutability is useful to ensure image integrity and can prevent supply chain attacks.

Closes aws#4640
add tag immutability example in description
In earlier commits, the imageTagMutability prop was set to MUTABLE by default, this commit reverts that change and fixes relevant tests
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I'm catching up from some vacation time.

I'm wondering if you have a specific use case for adding the value as a property to the Repository instance? Adding support to specify mutability in props makes sense, but I wonder if its useful to add the property to the construct?

@aws/aws-ecs-devx may have some input.

Comment on lines 446 to 451
// repository image tag mutability
if (props.imageTagMutability !== undefined) {
this.imageTagMutability = props.imageTagMutability;
resource.imageTagMutability = props.imageTagMutability;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we should just move passing this property to the L1 resource up into the constructor. Like

       const resource = new CfnRepository(this, 'Resource', {
      repositoryName: this.physicalName,
      // It says "Text", but they actually mean "Object".
      repositoryPolicyText: Lazy.anyValue({ produce: () => this.policyDocument }),
      lifecyclePolicy: Lazy.anyValue({ produce: () => this.renderLifecyclePolicy() }),
      imageTagMutability: props.imageTagMutability,
    });

If its undefined the property won't be included in synth output.

Copy link
Contributor Author

@ap00rv ap00rv Oct 15, 2020

Choose a reason for hiding this comment

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

agreed, will make this change soon, thanks.

@MrArnoldPalmer MrArnoldPalmer requested review from a team and kohidave and removed request for a team October 14, 2020 23:58
Merge Master from aws/aws-cdk v1.68.0
@gitpod-io
Copy link

gitpod-io bot commented Oct 17, 2020

@mergify mergify bot dismissed MrArnoldPalmer’s stale review October 17, 2020 17:16

Pull request has been modified.

@jmortlock jmortlock mentioned this pull request Nov 7, 2020
2 tasks
@ap00rv
Copy link
Contributor Author

ap00rv commented Nov 10, 2020

requesting update here :) thank you.

MrArnoldPalmer
MrArnoldPalmer previously approved these changes Dec 7, 2020
@mergify
Copy link
Contributor

mergify bot commented Dec 7, 2020

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).

@vsetka
Copy link

vsetka commented Jan 21, 2021

So, is this happening or?

@MrArnoldPalmer MrArnoldPalmer removed the request for review from kohidave February 19, 2021 21:53
@rebelzach
Copy link

Looks like this needs to have the conflicts resolved, then it could go in?

@MrArnoldPalmer
Copy link
Contributor

oh whoops I missed that this wasn't auto-merging successfully. Thanks for the pings @vsetka @rebelzach I'll work on updating this.

@mergify mergify bot dismissed MrArnoldPalmer’s stale review March 9, 2021 17:39

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Mar 9, 2021

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

This was referenced Mar 17, 2021
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.

[Python] ECR's "tag immutability" property not available?
5 participants