-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Support project-level Terraform distribution selection #5167
base: main
Are you sure you want to change the base?
Conversation
Add support for terraform_distribution config value in project config. This config value behaves similarly to terraform_version whereby defaults taken from the server config will be overridden by project-level values. Also refactor to prevent terraform client slightly to prevent cyclical dependencies. Signed-off-by: Andrew Borg <[email protected]>
@abborg thanks for the contribution, please gives us some time to review. |
@abborg, can you update the relevant documentation in the |
Signed-off-by: Andrew Borg <[email protected]>
I've updated the relevant documentation. Please let me know if there's any further changes desired. |
Signed-off-by: Andrew Borg <[email protected]>
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.
Looking good @abborg. Just a few suggestions.
cmd/server.go
Outdated
// TFDistributionFlag is deprecated for DefaultTFDistributionFlag | ||
TFDistributionFlag = "tf-distribution" | ||
TFDownloadFlag = "tf-download" | ||
TFDownloadURLFlag = "tf-download-url" | ||
UseTFPluginCache = "use-tf-plugin-cache" | ||
VarFileAllowlistFlag = "var-file-allowlist" | ||
VCSStatusName = "vcs-status-name" | ||
IgnoreVCSStatusNames = "ignore-vcs-status-names" | ||
TFEHostnameFlag = "tfe-hostname" | ||
TFELocalExecutionModeFlag = "tfe-local-execution-mode" | ||
TFETokenFlag = "tfe-token" | ||
WriteGitCredsFlag = "write-git-creds" // nolint: gosec | ||
WebBasicAuthFlag = "web-basic-auth" | ||
WebUsernameFlag = "web-username" | ||
WebPasswordFlag = "web-password" | ||
WebsocketCheckOrigin = "websocket-check-origin" |
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.
// TFDistributionFlag is deprecated for DefaultTFDistributionFlag | |
TFDistributionFlag = "tf-distribution" | |
TFDownloadFlag = "tf-download" | |
TFDownloadURLFlag = "tf-download-url" | |
UseTFPluginCache = "use-tf-plugin-cache" | |
VarFileAllowlistFlag = "var-file-allowlist" | |
VCSStatusName = "vcs-status-name" | |
IgnoreVCSStatusNames = "ignore-vcs-status-names" | |
TFEHostnameFlag = "tfe-hostname" | |
TFELocalExecutionModeFlag = "tfe-local-execution-mode" | |
TFETokenFlag = "tfe-token" | |
WriteGitCredsFlag = "write-git-creds" // nolint: gosec | |
WebBasicAuthFlag = "web-basic-auth" | |
WebUsernameFlag = "web-username" | |
WebPasswordFlag = "web-password" | |
WebsocketCheckOrigin = "websocket-check-origin" | |
TFDistributionFlag = "tf-distribution" // deprecated for DefaultTFDistributionFlag | |
TFDownloadFlag = "tf-download" | |
TFDownloadURLFlag = "tf-download-url" | |
UseTFPluginCache = "use-tf-plugin-cache" | |
VarFileAllowlistFlag = "var-file-allowlist" | |
VCSStatusName = "vcs-status-name" | |
IgnoreVCSStatusNames = "ignore-vcs-status-names" | |
TFEHostnameFlag = "tfe-hostname" | |
TFELocalExecutionModeFlag = "tfe-local-execution-mode" | |
TFETokenFlag = "tfe-token" | |
WriteGitCredsFlag = "write-git-creds" // nolint: gosec | |
WebBasicAuthFlag = "web-basic-auth" | |
WebUsernameFlag = "web-username" | |
WebPasswordFlag = "web-password" | |
WebsocketCheckOrigin = "websocket-check-origin" |
Reduce the diff.
@@ -73,6 +73,7 @@ var testFlags = map[string]interface{}{ | |||
CheckoutStrategyFlag: CheckoutStrategyMerge, | |||
CheckoutDepthFlag: 0, | |||
DataDirFlag: "/path", | |||
DefaultTFDistributionFlag: "terraform", |
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 add a ValidateDefaultTFDistribution
test?
server/core/config/raw/project.go
Outdated
func validDistribution(value interface{}) error { | ||
distribution := value.(*string) | ||
if distribution != nil && *distribution != "terraform" && *distribution != "opentofu" { | ||
return fmt.Errorf("%q is not a valid terraform_distribution, only %q and %q are supported", *distribution, "terraform", "opentofu") |
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.
return fmt.Errorf("%q is not a valid terraform_distribution, only %q and %q are supported", *distribution, "terraform", "opentofu") | |
return fmt.Errorf("'%s' is not a valid terraform_distribution, only '%s' and '%s' are supported", *distribution, "terraform", "opentofu") |
%q
produces a double quoted string which then needs to be escaped in the JSON log output. Single quoted strings are better as they don't have to be escaped.
@@ -39,19 +41,27 @@ func (a *ApplyStepRunner) Run(ctx command.ProjectContext, extraArgs []string, pa | |||
|
|||
ctx.Log.Info("starting apply") | |||
var out string | |||
tfDistribution := a.DefaultTFDistribution | |||
if ctx.TerraformDistribution != nil { |
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.
Is there a test that covers this condition? Same question for all runners.
@@ -14,7 +14,7 @@ | |||
// Modified hereafter by contributors to runatlantis/atlantis. | |||
// | |||
// Package terraform handles the actual running of terraform commands. |
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.
// Package terraform handles the actual running of terraform commands. | |
// Package tfclient handles the actual running of terraform commands. |
"github.com/runatlantis/atlantis/server/core/terraform" | ||
|
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.
"github.com/runatlantis/atlantis/server/core/terraform" | |
"github.com/runatlantis/atlantis/server/core/terraform" |
Remove blank line
server/user_config.go
Outdated
// TFDistribution is deprecated in favor of DefaultTFDistribution | ||
TFDistribution string `mapstructure:"tf-distribution"` |
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.
// TFDistribution is deprecated in favor of DefaultTFDistribution | |
TFDistribution string `mapstructure:"tf-distribution"` | |
TFDistribution string `mapstructure:"tf-distribution"` // deprecated in favor of DefaultTFDistribution |
Reduce diff
There are some test failures too: https://github.com/runatlantis/atlantis/actions/runs/12394308257/job/34599029347?pr=5167 |
@X-Guardian I should be able to apply your suggestions and fix remaining test failures later today. |
Edit: I was able to checkout the previous head, make the correct signoff and then push the changes. |
Signed-off-by: Andrew Borg <[email protected]>
Signed-off-by: Andrew Borg <[email protected]>
It looks like this relates to the OpenTofu epic #3741 - does this also provide a resolution to Support OpenTofu auto download #4339? (I'm not one of the Atlantis developers, just an interested Atlantis user starting to experiment with OpenTofu - and this is very exciting to see!) |
what
terraform_distribution
project config valueterraform_distribution
config value overrides server defaultwhy
tests
references
Supports #3741