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

[AS8-6579] Initial checkin #1

Merged
merged 8 commits into from
Jul 12, 2021
Merged

[AS8-6579] Initial checkin #1

merged 8 commits into from
Jul 12, 2021

Conversation

cjonesy
Copy link
Contributor

@cjonesy cjonesy commented Jun 11, 2021

This module is mostly based almost exactly on the instructions found in GCP's Configuring Slack notifications.

Since there was a significant amount of Terraform code this felt like it should be a standalone Terraform module that can be pulled in to any other module when you want to enable Slack notifications. I also believe we could open source this and push it up to the Terraform Module Registry, so separating it into another repo makes that easier.

@cjonesy cjonesy requested review from a team, ruffin, jason-h-35 and joshuawscott and removed request for a team June 11, 2021 18:36
main.tf Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
@cjonesy
Copy link
Contributor Author

cjonesy commented Jun 11, 2021

FYI - The name of this repo is likely to change to be something that aligns with most of the other community modules. terraform-module-<thing> or terraform-<thing>.

variables.tf Outdated Show resolved Hide resolved
Copy link
Member

@joshuawscott joshuawscott left a comment

Choose a reason for hiding this comment

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

LGTM so far

@cjonesy
Copy link
Contributor Author

cjonesy commented Jul 8, 2021

I realized I had been churning on this module for a while now and figured it would be better to just get it out the door with the existing Slack message formatting (even though they are very lacking).

I am hoping that this issue gets handled soon and we can have better info in the messages. However, even if that issue is not resolved, I can build an app that does what we want and override the image with cloud_build_notifier_image.

main.tf Outdated
provider = google-beta

#FIXME: Is there no way to tell a cloud run service to restart when it's config changes?
name = "${local.base_name}-${lower(regex("[0-9A-Za-z]+", google_storage_bucket_object.cloud_build_notifier_config.crc32c))}" # HACK To make the cloud run job change when the config changes
Copy link
Contributor Author

@cjonesy cjonesy Jul 8, 2021

Choose a reason for hiding this comment

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

This feels really gross to me, but I can't find a better way to handle this.

When we make a change to the configuration in a Cloud Run service it doesn't notice and just continues to run with the old configuration.

I read through all the options in the google_cloud_run_service docs, but could not find anything to trigger it to deploy a new revision on config change.

The only way I could come up with is appending the configs crc32 to the end of the service's name... which triggers terraform to destroy and recreate the service.

If anyone has ideas on how to get around this I'd love to hear them!

Copy link
Member

Choose a reason for hiding this comment

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

hashicorp/terraform#11418
It seems like this approach is the best way (assuming it actually redeploys when the name changes?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I verified that it does redeploy when the config changes.

# ------------------------------------------------------------------------------

# Create cloud build notifier service account
resource "google_service_account" "notifier" {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to output this

Copy link
Member

Choose a reason for hiding this comment

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

never mind on this, I was confused on what it was used for.

Copy link
Member

@joshuawscott joshuawscott left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

@jason-h-35 jason-h-35 left a comment

Choose a reason for hiding this comment

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

What's the motivation behind using the 1-repo-1-module pattern vs. putting it in simplifi/terraform-modules? Just the potential to open-source it later?

@cjonesy
Copy link
Contributor Author

cjonesy commented Jul 12, 2021

What's the motivation behind using the 1-repo-1-module pattern vs. putting it in simplifi/terraform-modules? Just the potential to open-source it later?

@jason-h-35 - Yep! This module felt generic enough that we could open-source it and push it to terraform registry.

@cjonesy cjonesy merged commit e3c6896 into main Jul 12, 2021
@cjonesy cjonesy deleted the init branch July 12, 2021 18:37
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.

4 participants