-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Preview env] TF Workflow #12981
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
[Preview env] TF Workflow #12981
Conversation
3f98ce0
to
5bc1456
Compare
e7d0266
to
a2ec2c5
Compare
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.
Really awesome to see the first steps for hits 🎉 I have some high-level comments.
Written in bash, so they can be executed from any environment and have dependency solely on terraform binary being available.
I was really hoping to reduce the amount of bash we have for critical components. I'd be okay having two system dependencies of "go and terraform" just to avoid adding more bash. But that's my personal preference.
I didn't dive into the bash implementation in this review as I wanted to discuss this first
This is currently introducing a new top-level folder named preview. Previously we have put everything related to our "dev experience" in ./dev and we already have a ./dev/preview folder so I'd prefer to move everything there. So that would be:
dev
preview
previewctl
infrastructure
workflow
terraform { | ||
|
||
backend "gcs" { | ||
bucket = "3f4745df-preview-tf-state" |
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.
What's 3f4745df
?
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.
It's a random string - bucket names are public and globally unique, so I prefer to add some randomness to each.
} | ||
} | ||
|
||
# https://registry.terraform.io/providers/harvester/harvester/latest/docs |
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 know we link to the docs in many places, but I find it bit redundant so would prefer to not have them unless there's a specific reason for it.
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.
👍
provider "k8s" { | ||
alias = "harvester" | ||
config_path = var.harvester_kube_path | ||
} |
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.
How come our pre-commit hook end-of-file-fixer
didn't remove these missing newlines?
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.
Speaking of pre-commit-hooks it would be nice to add one for terraform fmt https://github.com/antonbabenko/pre-commit-terraform#3-add-configs-and-hooks
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.
Ah I commit with --no-verify
by default and usually at the end I run the pre-commit manually to fix anything outstanding 😅 Weird that we don't run the pre-commit as a step in the CI so it fails in cases like this.
I'll add the terraform fmt
in another PR with the rest of the TF.
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 was really hoping to reduce the amount of bash we have for critical components. I'd be okay having two system dependencies of "go and terraform" just to avoid adding more bash. But that's my personal preference.
IMO wrapping terraform
in be it go
, or ts
, or anything else makes very little (next to none) sense. I've purposefully written it in bash
as it's quite a thin layer (albeit a little opinionated), and you can run it from anywhere and requires only terraform
to exist. As we discussed, this is also a foundation that we'll be using for access
, and ops
- and I would probably separate it into a dedicated Action once we get to that point.
This is currently introducing a new top-level folder named preview. Previously we have put everything related to our "dev experience" in ./dev and we already have a ./dev/preview folder so I'd prefer to move everything there. So that would be:
👍 I don't have a strong opinion on that. Will move it in a bit.
} | ||
} | ||
|
||
# https://registry.terraform.io/providers/harvester/harvester/latest/docs |
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.
👍
provider "k8s" { | ||
alias = "harvester" | ||
config_path = var.harvester_kube_path | ||
} |
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.
Ah I commit with --no-verify
by default and usually at the end I run the pre-commit manually to fix anything outstanding 😅 Weird that we don't run the pre-commit as a step in the CI so it fails in cases like this.
I'll add the terraform fmt
in another PR with the rest of the TF.
328ec90
to
710739c
Compare
That's fair, if we don't expect these scripts to grow beyond just invoking a bit of TF I agree it's overkill to start using a "proper language" for it. |
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 added the hold label so you can update the CODEOWNERS but otherwise this looks like a great start to me. Would love a demo on during the Platform Sync on Monday.
.github/CODEOWNERS
Outdated
@@ -100,6 +100,9 @@ | |||
/.werft/*installer-tests* @gitpod-io/engineering-self-hosted | |||
/.werft/jobs/build/self-hosted-* @gitpod-io/engineering-self-hosted | |||
|
|||
/preview/infrastructure/harvester @gitpod-io/platform |
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.
We can remove these now that the files have been moved as we already own /dev/preview
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've updated them, but I would like to keep them explicit, as we would like to separate them in the future.
7d70e55
to
f364419
Compare
@vulkoingim the build is failing so added the hold label |
f364419
to
47a6f15
Compare
Description
terraform
binary being available.[init, plan, apply, workspace]
, so they can be executed in any context.Other PRs will follow with werft job and harvester VM will follow.
Related Issue(s)
Fixes https://github.com/gitpod-io/ops/issues/5093
How to test
Release Notes
Documentation
Werft options: