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

Upgrade to Terraform 1.0.11 #228

Closed

Conversation

mtarsel
Copy link

@mtarsel mtarsel commented Dec 2, 2021

Removes the terraform required_version variable from all modules. I went with this version because it was available on brew when I did a fresh install on my Mac.

Install instructions

Upgrade instructions

Removes the terraform required_version variable from all modules.

Install instructions:
https://learn.hashicorp.com/tutorials/terraform/install-cli

Signed-off-by: Mick Tarsel <mtarsel@gmail.com>
@ppc64le-cloud-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtarsel
To complete the pull request process, please assign yussufsh after the PR has been reviewed.
You can assign the PR to them by writing /assign @yussufsh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ppc64le-cloud-bot
Copy link

Welcome @mtarsel! It looks like this is your first PR to ocp-power-automation/ocp4-upi-powervm 🎉

@ppc64le-cloud-bot
Copy link

Hi @mtarsel. Thanks for your PR.

I'm waiting for a ocp-power-automation member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cs-zhang
Copy link
Collaborator

cs-zhang commented Dec 9, 2021

@yussufsh Do you know why we need to add the TF version to all sub-modules version file?

@yussufsh
Copy link
Contributor

yussufsh commented Dec 9, 2021

Not needed but if someone is using the modules then he can use any versions and then will complain. Eg: https://github.com/ocp-power-automation/infra/tree/master/infra-node

@yussufsh
Copy link
Contributor

yussufsh commented Dec 9, 2021

@mtarsel we wanted to stick to a particular version and everyone uses the same one so we don't have to investigate on issues that are Terraform version related.
We can however start using Terraform version >1.0 but it needs proper verification on all scenarios. I hope @cs-zhang and team can handle the verification part.

@cs-zhang cs-zhang requested a review from aishwaryabk December 9, 2021 18:02
@cs-zhang
Copy link
Collaborator

cs-zhang commented Dec 9, 2021

@aishwaryabk can you test this one and the PowerVS with new TF version?

versions.tf Outdated Show resolved Hide resolved
@aishwaryabk
Copy link
Collaborator

@aishwaryabk can you test this one and the PowerVS with new TF version?

Yes, sure.

Signed-off-by: Mick Tarsel <mtarsel@gmail.com>
@cs-zhang
Copy link
Collaborator

@aishwaryabk has found bug for TF 1.1.0, so we have to limit the version to 1.0.x, the version should be required_version = "~> 1.0.0". Also don't remove TF version from sub-modules.

@mtarsel
Copy link
Author

mtarsel commented Dec 13, 2021

@aishwaryabk is there an issue filed on this repo we can reference in this PR and mention in the commit?

I have no problem adding the version restrictions with required_version = "~> 1.0.0" but would like to understand why.

Also don't remove TF version from sub-modules.

@cs-zhang shouldn't the modules all use the same TF versions? Is there any reason for 1 module to use a different version than the rest of the modules? I assumed the main versions.tf was inherited by all the modules. I am new to TF but curious why this is needed. Thanks!

@aishwaryabk
Copy link
Collaborator

@aishwaryabk is there an issue filed on this repo we can reference in this PR and mention in the commit?

I have no problem adding the version restrictions with required_version = "~> 1.0.0" but would like to understand why.

Also don't remove TF version from sub-modules.

@cs-zhang shouldn't the modules all use the same TF versions? Is there any reason for 1 module to use a different version than the rest of the modules? I assumed the main versions.tf was inherited by all the modules. I am new to TF but curious why this is needed. Thanks!

@mtarsel Currently, there is no issue filed. This is a TF bug and will require more investigation.

@cs-zhang
Copy link
Collaborator

cs-zhang commented Apr 5, 2022

We already made the change to use any TF version which great than 1.0. So this PR is no longer needed and close it.

@cs-zhang cs-zhang closed this Apr 5, 2022
@yussufsh
Copy link
Contributor

yussufsh commented Apr 5, 2022

We already made the change to use any TF version which great than 1.0. So this PR is no longer needed and close it.

I don't think the change is done for this project.

@cs-zhang
Copy link
Collaborator

cs-zhang commented Apr 5, 2022

Here is the PR: #242

@yussufsh
Copy link
Contributor

yussufsh commented Apr 5, 2022

Here is the PR: #242

👍
I will update the docs soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants