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: add tf-account to support multiple customers #1

Merged
merged 127 commits into from
Sep 14, 2023

Conversation

obs-gh-nikhildua
Copy link
Collaborator

@obs-gh-nikhildua obs-gh-nikhildua commented Aug 21, 2023

Description

  • Borrowed from here to support applying tf-account-* to multiple customers
  • Additional customer is passed in as an optional input to workflow (additional_customers)
  • This additional customer gets mapped to be a new workspace.
  • Plan comments has been updated
  • New workflow step to create /select workspaces and configure the correct backend
  • Backend for non-default: -backend-config="workspace_key_prefix=${{ github.event.repository.name }}"
    which gets mapped in the bucket as <repo_name>/<workspace>/<key> where key is <repo_name>/tf-state
Screenshot 2023-08-21 at 10 34 53 AM

Note: For terraform to apply this workspace, additional customer MUST have a mapping present in the tf-account provider.tf!
See: https://github.com/observeinc/tf-account-dual-terraform/pull/2/files

Testing

locals {
  workspace_to_customer_map = {
    default = local.secrets["OBSERVE_CUSTOMER"]   # Default customer for tf-account 
    cap2    = local.secrets["OBSERVE_CUSTOMER_2"] # Additional customer for tf-account
  }

  customer = lookup(local.workspace_to_customer_map, terraform.workspace)

}

@obs-gh-nikhildua obs-gh-nikhildua force-pushed the nikhil/tf-pipeline branch 2 times, most recently from 81e4fe5 to a6db2c5 Compare August 21, 2023 17:22
@obs-gh-nikhildua obs-gh-nikhildua marked this pull request as ready for review August 21, 2023 17:45
Copy link

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

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

Good start. Key feedback here is that this should support n customers, not n=2.

For terraform to apply this workspace, additional customer MUST have a mapping present in the tf-account provider.tf!

Let's pare back the misuse of secrets instead of creating extra indirection. You should not have to touch your Terraform configuration whatsoever to get this working or make it aware of workspaces. The workflow should handle passing in the right values.

.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
@bendrucker
Copy link

Avoid force push. It's a bad habit we've ingrained in people via workflows all over worry that a trivial config (squash and merge) won't be enforced reliably in all calling repos. Outside of observeinc/terraform-observe-* repos that enforce otherwise, it's good PR etiquette to use commits to provide a timeline of meaningful changes. Not too small but not too large. Rewriting and squashing or amending commits in a PR (vs on merge) prevents reviewers from seeing that history.

@obs-gh-nikhildua
Copy link
Collaborator Author

obs-gh-nikhildua commented Sep 12, 2023

@bendrucker @jta
See: https://github.com/observeinc/tf-account-o2/pull/1 for first use of this PR to "setup" foundation for O2

I'd like to get this merged fairly soon and iterate on it later if needed, as I'll have to switch priorities shortly.

Copy link

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

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

Nice! All should be quick fixes but feel free to merge this and follow up.

.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
.github/workflows/tf-account.yaml Outdated Show resolved Hide resolved
@obs-gh-nikhildua obs-gh-nikhildua merged commit 04e9ce3 into main Sep 14, 2023
@obs-gh-nikhildua obs-gh-nikhildua deleted the nikhil/tf-pipeline branch September 14, 2023 20:52
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.

2 participants