-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add architecture diagram and example module usage #25
Conversation
README.md
Outdated
|
||
# ECR variables | ||
container_name = "github-runner" | ||
allowed_read_principals = local.principals_ro_access |
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.
Where is this defined?
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.
updated for this to fall more in-line with being a registered module
README.md
Outdated
# ECR variables | ||
container_name = "github-runner" | ||
allowed_read_principals = local.principals_ro_access | ||
ci_user_arn = data.aws_iam_user.github_ci_user.arn |
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 include the example of how this is defined?
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.
just added to the docs for all of these, thanks for pointing this out
|
||
# ECS variables | ||
environment = "dev" | ||
ecs_desired_count = 0 |
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.
Maybe it makes sense to remove ecs_desired_count
as a variable now that the count is being managed by the actions workflow
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.
I think it's still worth having the flexibility. it is optional, and with the updated docs, it's more clear it's optional/has a default of 0
README.md
Outdated
# ECS variables | ||
environment = "dev" | ||
ecs_desired_count = 0 | ||
ecs_vpc_id = data.aws_vpc.example_east_sandbox.id |
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 include example of how the VPC is being looked up as a data source?
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.
I ended up making this usage more generic, but did add a section on how to use data sources to look-up info in AWS
README.md
Outdated
logs_cloudwatch_group_arn = aws_cloudwatch_log_group.main.arn | ||
|
||
# GitHub Runner variables | ||
personal_access_token_arn = data.aws_secretsmanager_secret_version.token.arn |
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 include example of how the secret is being looked up as a data source?
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.
yup, added under data source example section
|
This PR adds: