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!: Allow custom ECR, rename var.use_ecr_image, and cleanup #51

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

baolsen
Copy link
Contributor

@baolsen baolsen commented Dec 10, 2024

Closes #48

Breaking change, as I decided to change one of the variables a bit to support more use cases.

The plan is to test this branch at the customer before merging it in.

Copy link

infracost bot commented Dec 10, 2024

💰 Infracost report

Monthly estimate generated

This comment will be updated when code changes.

@baolsen baolsen force-pushed the 2024-12-10-housekeeping branch 2 times, most recently from cddef81 to d85ec1e Compare December 10, 2024 09:26
@baolsen baolsen changed the title feat: Support custom ECR image, and cleanup a few things feat!: Allow custom ECR image, rename use_ecr_image variable, and cleanup a few things Dec 10, 2024
@baolsen baolsen force-pushed the 2024-12-10-housekeeping branch from d85ec1e to ce86705 Compare December 10, 2024 09:28
@baolsen baolsen changed the title feat!: Allow custom ECR image, rename use_ecr_image variable, and cleanup a few things feat!: Allow custom ECR, rename var.use_ecr_image, and cleanup Dec 10, 2024
@baolsen baolsen requested a review from andrewchees December 10, 2024 09:29
@@ -2,6 +2,7 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved some sections around to make it easier to read

@@ -57,59 +58,22 @@ For example:

# How to use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the auth steps into their own file


There are several ways to set up authentication to GitHub.
Follow the guide [here](/docs/GITHUB-AUTH-SETUP.md).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before merging I probably need to make this a URI like the others. Or it wont work from the TF registry.

@@ -13,12 +13,6 @@ locals {

subnet_arns = [for subnet_id in var.subnet_ids : "arn:aws:ec2:${local.aws_region}:${local.aws_account_id}:subnet/${subnet_id}"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this closer to where the action happens

local.create_iam_role
? aws_iam_role.this[0].arn
: "arn:aws:iam::${local.aws_account_id}:role/${var.iam_role_name}"
)

artifacts {
type = "NO_ARTIFACTS"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tweaked the logic a bit

@baolsen
Copy link
Contributor Author

baolsen commented Dec 10, 2024

I'll also add a precommit hook for codespell for completeness

@baolsen baolsen force-pushed the 2024-12-10-housekeeping branch from a871c12 to 8113e44 Compare December 10, 2024 13:49
@baolsen baolsen merged commit 975cb80 into main Dec 11, 2024
10 checks passed
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.

Ability to specify custom ECR image
2 participants