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

Set runner.InitRequest.Upgrade to false by default #1042

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hmonsalv
Copy link

Prevents terraform runner to upgrade all providers, and will respect the definition of the terraform lock file if any. If required, this could be parametrized, so that the user can decide whether to enable upgrade option or not.

Prevents terraform runner to upgrade all providers, and will respect the definition of the terraform lock file if any. If required, this could be parametrized, so that the user can decide whether to enable upgrade option or not.
@chanwit
Copy link
Collaborator

chanwit commented Oct 11, 2023

@hmonsalv thank you for this PR.

Would you like to try adding a new field to the CR?
Maybe like this:

kind: Terraform
spec:
  upgrade: false # default to true

The new upgrade flag would still be true by default to retrain compatibility. We would come back later for example in v1alpha3 or v1beta1 to set this value to false by default.

@chanwit
Copy link
Collaborator

chanwit commented Oct 11, 2023

Here's another field, RefreshBeforeApply, which provides a flag but for the apply phase: https://github.com/weaveworks/tf-controller/blob/main/api/v1alpha2/terraform_types.go#L214

@defreng
Copy link

defreng commented Oct 11, 2023

Thanks @hmonsalv for this!

Actually @chanwit, do you think it's important to maintain backwards compatibility for this? To me it seems like the current behaviour is more like a bug, where tf-controller incorrectly upgrades providers while this should never be done automatically?

@chanwit
Copy link
Collaborator

chanwit commented Oct 11, 2023

Make sense!

If many agreed, we could introduce this as a breaking change + docs by having the upgrade field default to false.

Then if users wanted the old behavior, they would go setting it to true.

@defreng would you be able to help with the docs part for this PR?

@defreng
Copy link

defreng commented Oct 26, 2023

Hi @chanwit

mmh - not so sure... I guess the relevant documentation in upstream terraform is this one: https://developer.hashicorp.com/terraform/language/files/dependency-lock

@yitsushi
Copy link
Collaborator

yitsushi commented Jan 5, 2024

Can you please add the user configurable field in the spec?

As far as I see the correct behaviour would be to NOT upgrade by default. The whole purpose of the controller is to maintain consistency with the git repository (gitops) and prevent most events that would result deviation from the source.

(also docs 😆)

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.

None yet

4 participants