Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Implement CodeArtifact L2 construct #11091
feat: Implement CodeArtifact L2 construct #11091
Changes from all commits
c8e209a
915e8d0
d2e98db
51cc8b6
a51709f
be5460d
4109da9
4b2e38c
c4810a8
afbc8d1
c2749db
bc6629b
73dae98
c6309b2
abe91a4
7ff616c
1901b74
2b056f9
786329f
6a158db
b6d7193
a4a0247
a6e4785
5f4a126
bd212b2
d8442b4
91648bf
4489488
2ba3655
fbde98d
c714c5d
07ed06b
19cbc00
dc7ca1a
070c4c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md#factories again. Ideally, this
addRepository
method takes in aRepositoryOption
interface, which is a sub-interface ofRepositoryProps
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a sentence (or two) here (and for Upstream Repositories) just explaining what they are? Also, I still think we should defer
withExternalConnections
until we have a use case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, users should either (1) specify just the
principal
and get the default permissions; (2) should specify thepolicyDocument
with the policy they want; or (3) specify neither and get the default permissions on the account as a whole? If so, can you make that a bit clearer in the docstrings? Maybe something like this for theprincipal
?Then we might even want a check (and error) in the constructor are both specified.
Edit: Actually, looking at this again (after almost finishing the review), I'm wondering if we want this at all (both here, and for
Repository
). By default -- without specifying a policy -- the account root will have all access to theDomain
, so creating this default policy is actually a no-op. At least according to the docs, the statement is optional and we can omit it for that default case. The only time a domain resource policy is needed is for cross-account access.Here's what I'd propose: let's remove this parameter for now. We can add it in as a non-breaking change later. If a policy document is specified, great, we can use it. Otherwise, we leave the parameter undefined. If a user calls one of the
grant
methods and a policy document doesn't yet exist, we can create it at that point. Make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove, per the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
''
evaluates as false in Javascript/Typescript.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just inline set these values. Additionally, you can inline the class definition altogether:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should set the
domainOwner
from the ARN as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to initialize these variables; they're initialized in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this private for now -- unless there's a use case for exposing it. Between the
Props
and thegrant
methods, we should be able to cover 99.9% of use cases with this private.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See README comment for how this API definition should be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D I just meant to include the doc link as a comment, so future maintainers can update it appropriately. Including it in the
validate
method isn't strictly necessary. Also, you'll find -- once you create the eslint config -- that this line is too long.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're never saving this value (
this.policyDocument
will never be set).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add the statement if the document already exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second read, I think
grantLogin
isn't quite intuitive. In the case of "neither name is great", I'd prefer we use the service's terminology.