feat: support passing multiple trustedRepositories#41
Conversation
|
SonarCloud Quality Gate failed.
|
| readonly trustedRepositories?: TrustedRepository[]; | ||
| } | ||
|
|
||
| export interface TrustedRepository { |
There was a problem hiding this comment.
TrustedRepository takes the same top-level owner/repo and optionally filter props.
| * | ||
| * @default - required if top-level owner/repo/filter not set | ||
| */ | ||
| readonly trustedRepositories?: TrustedRepository[]; |
There was a problem hiding this comment.
Up for discussion - would it be clearer to deprecate / remove the top-level properties and force people to use trustedRepostories instead?
|
|
||
| if (!trustedRepositoriesSet && (props.owner === undefined || props.repo === undefined)) { | ||
| cdk.Annotations.of(scope).addError("If you don't provide `trustedRepositories`, you must provide `owner` and `repo`."); | ||
| } |
There was a problem hiding this comment.
If we decided to deprecate the top-level properties, we could annotate the deprecated properties here.
| ...roleProps, | ||
| assumedBy: new iam.WebIdentityPrincipal(provider.openIdConnectProviderArn, { | ||
| StringLike: { | ||
| 'ForAnyValue:StringLike': { |
There was a problem hiding this comment.
This should be safe even if just one property is passed. From the docs:
ForAnyValue – This qualifier tests whether at least one member of the set of request context key values matches at least one member of the set of context key values in your policy condition. The context key returns true if any one of the context key values in the request matches any one of the context key values in the policy. For no matching context key or a null dataset, the condition returns false.
If we think people might be worried about seeing a cdk diff with this, we could optionally use ForAnyValue only when multiple repos are passed.
There was a problem hiding this comment.
When setting this up manually, StringLike also worked. As there is only one key in this map there should be no difference. The array of values is automatically evaluated as an OR
|
Quality gate seems to be complaining about the tests having duplication... |
|
Hey @aripalo - any chance we can get this reviewed? If this isn't the direction you want to go with this repo, I might fork and publish a separate package. TYIA for your time and this great project! |
|
@blimmer, did you publish a fork after all? I would be interested in using it. |
|
Hey @oxc. I didn't publish a fork. I'd really like to see if @aripalo would be open to integrating this behavior to have a single canonical project for this purpose. However, if they don't have bandwidth to review the PR, I can look into publishing a namespaced fork (e.g., |
|
@aripalo we can also consider integrating this within the https://github.com/open-constructs/aws-cdk-library |
|
Since I haven't received a response to this PR for quite some time, I've decided to create my own replacement construct for this library: https://github.com/blimmer/cdk-github-oidc. My library fixes this issue and all other outstanding issues for this repo. @aripalo, please feel free to archive this repo and point people at mine if you'd like. |
|
3 participants
This PR introduces a new API that allows passing multiple subjects via the
trustedRepositoriesproperty.Fixes #35