feat: add paths #787
feat: add paths #787
Conversation
| @@ -206,7 +206,7 @@ def deploy_module(mdo: ModuleDeployObject) -> ModuleDeploymentResponse: | |||
| + store_sf_bundle, | |||
| extra_env_vars=env_vars, | |||
| codebuild_compute_type=module_manifest.deploy_spec.build_type, | |||
| codebuild_role_name=mdo.module_role_name, | |||
| codebuild_role_name=mdo.module_role_arn, | |||
There was a problem hiding this comment.
Is the change implemented so that the role path is included?
There was a problem hiding this comment.
Correct. Path is part of the ARN. This feeds into CodeBuild's StartBuild API, serviceRoleOverride.
| policy = yaml.safe_load(f) | ||
|
|
||
| if policy_prefix: | ||
| policy["Resources"]["ProjectPolicy"]["Properties"]["Path"] = policy_prefix |
There was a problem hiding this comment.
This is similar to bootstrap template overrides, for example here. I could introduce jinja instead but this is an overkill for one parameter override imo.
| @@ -54,23 +54,23 @@ Resources: | |||
| - logs:GetLogGroupFields | |||
| - logs:GetQueryResults | |||
| Resource: | |||
| - !Sub "arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}:log-group:/aws/codebuild/codeseeder-${ProjectName}*" | |||
| - Fn::Sub: "arn:${AWS::Partition}:logs:${AWS::Region}:${AWS::AccountId}:log-group:/aws/codebuild/codeseeder-${ProjectName}*" | |||
There was a problem hiding this comment.
Fn::Sub is consistent with our bootstrap templates and can be processed by any yaml parser. Just to not have to bring cfn-flip as a dependency.
| "security_group_ids": security_group_ids, | ||
| } | ||
|
|
||
| if version.parse(cs.__version__) >= version.parse("1.3.0"): |
There was a problem hiding this comment.
We need to cut a codeseeder release and then will be able to remove this.
There was a problem hiding this comment.
Does codeseeder 1.3.0 already support this? Why not just bump the minimum version of code-seeder?
There was a problem hiding this comment.
We just didn't cut 1.3.0 yet and I wanted to make this runnable with older versions until then
seedfarmer/__main__.py
Outdated
| @click.option( | ||
| "--role-prefix", | ||
| default="/", | ||
| help="""An IAM path prefix to use with the seedfarmer roles. | ||
| Use only if bootstrapped with this path""", | ||
| required=False, | ||
| ) |
There was a problem hiding this comment.
The changes for apply, destroy, synth seem undocumented?
| - Fn::Sub: "arn:${AWS::Partition}:iam::*:role/seedfarmer-{{ project_name }}*" | ||
| - Fn::Sub: "arn:${AWS::Partition}:iam::*:role/*/seedfarmer-{{ project_name }}*" |
There was a problem hiding this comment.
Would it make more sense to use the prefix here for least privilege?
| - Fn::Sub: "arn:${AWS::Partition}:iam::*:role/seedfarmer-{{ project_name }}*" | |
| - Fn::Sub: "arn:${AWS::Partition}:iam::*:role/*/seedfarmer-{{ project_name }}*" | |
| - Fn::Sub: "arn:${AWS::Partition}:iam::*:role{{ role_prefix }}seedfarmer-{{ project_name }}*" |
Same goes for deployment_role.template.
There was a problem hiding this comment.
This is what I've started with but prefixes used with toolchain, deployment, and module deployment roles can be different (with the current design).
We can set it to one specific prefix here but that would essentially mean all prefixes must be the same across all roles (and more importantly all accounts, that may be owned by different teams with different requirements / SCP)
There was a problem hiding this comment.
Might make sense to leave a comment or something to that?
Issue #, if available: #757
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.