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

Update terraform-libvirt-provider #1023

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

Conversation

rjmateus
Copy link
Member

@rjmateus rjmateus commented Dec 27, 2021

We have to wait until the submissions of 4.3.1 before merging.

This PR will only be merged after agreement with QA team, since it can temporarily crash all test environments.

What does this PR change?

Updates sumaform to use terraform loibvirt provider latest provider from terraform registry.
We will be using the upstream provider since all patches were included in the latest released version.
We will keep the repository and provider for uyuni in case we need to add another patch in the future.

When this PR is merged, we need to:

  • Send email to the mailing list
  • Adapt CI terraform files to cope with the change: Update terraform-libvirt-provider SUSE/susemanager-ci#432
  • Clean CI environment to cope with the changes
  • Remove package from all our CI worker machines.
  • Update the terraform-provider-libvirt/terraform together with that in the OBS if necessary/wanted

How to upgrade

  • Un-install the provider's package zypper rm terraform-provider-libvirt
  • In you main.tf, change the provider version to 0.6.14
  • If you want to keep the same state file, see keeping state file. Otherwise just clan the environment and remove the terraform.tfstate, .terraform.lock.hcl and .terraform
  • Run terraform init

Keeping state file

  • Open theterraform.tfstate
  • Search for all cpu keys. The value should now be an array of objects. To change it just surround the existing content with square brackets ([ {....} ] )
  • Run terraform init -upgrade

Copy link
Member

@juliogonzalez juliogonzalez left a comment

Choose a reason for hiding this comment

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

Changes LGTM, but I fail to understand why you are asking to remove the old RPM instead of just updating. I guess it was already discussed with QA.

I think you will also need to send an email to the mailing lists about this, as it can crash environments for the developers.

@rjmateus
Copy link
Member Author

@juliogonzalez we need to remove because the way the plugin is provided will change.
With version 0.6.3 it was provided by a package built by us on OBS. For version 0.6.12 we are relying on the terraform registry service and getting the provider directly from the official location.

When this PR is merged, we need to:

  • Send email to mailing list
  • Adapte CI terraform files to cope with the cahnge (PR already opened)
  • Clean CI environment to cope with the changes
  • Remove package from al lour CI worker machines.

@juliogonzalez
Copy link
Member

@juliogonzalez we need to remove because the way the plugin is provided will change. With version 0.6.3 it was provided by a package built by us on OBS. For version 0.6.12 we are relying on the terraform registry service and getting the provider directly from the official location.

100% correct. I didn't remember we were working on this before the holidays :-D

When this PR is merged, we need to:

  • Send email to mailing list
  • Adapte CI terraform files to cope with the cahnge (PR already opened)
  • Clean CI environment to cope with the changes
  • Remove package from al lour CI worker machines.

PR LGTM. I am approving now. Feel free to merge as soon as QE is happy about it.

Copy link
Contributor

@ktsamis ktsamis left a comment

Choose a reason for hiding this comment

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

Looks good but let's schedule it please. We can do it on a Monday so we do the cleanups/removal/installation etc at the end of the day and start again all envs. That way we see on Tuesday if there is a problem.

@SchoolGuy
Copy link
Contributor

As per #1017 (comment) we should update this once again if we don't want to get trouble with our SSH Servers.

@nkrinner
Copy link

Some more context here: dmacvicar/terraform-provider-libvirt#916 (provieded by @nodeg )

Background: The current provider will not work with an updated ssh (from version 8.8) and machines will not be accessible.

@rjmateus rjmateus force-pushed the update_libvirt_0_6_12 branch from 2f41851 to 780cec5 Compare February 11, 2022 11:10
@rjmateus rjmateus changed the title update terraform libvirt provider ro 0.6.12 update terraform libvirt provider ro 0.6.14 Feb 11, 2022
@rjmateus rjmateus changed the title update terraform libvirt provider ro 0.6.14 update terraform libvirt provider to 0.6.14 Feb 11, 2022
@rjmateus
Copy link
Member Author

rjmateus commented Feb 11, 2022

@nkrinner I updated the PR to use the latest version of the provider (0.6.14), which should solve this problem according to the release notes [1].
Now I would say that we can merge as soon as QA gives the OK to merge it. I can prepare the release e-mail with the steps for all stakeholders to get a working sumaform with these changes.

[1] https://github.com/dmacvicar/terraform-provider-libvirt/releases/tag/v0.6.14

@srbarrios
Copy link
Member

Sorry to be that bad guy, but I think that starting Beta submissions, it might not be the right moment for risky changes or big refactors, I would wait until we release 4.3

@nodeg
Copy link
Member

nodeg commented Jun 10, 2022

Since 4.3 FCS is coming soon, we can give this another try after it.
I found dmacvicar/terraform-provider-libvirt#939 which seems a regression of 0.6.14. I have not tried it myself but if we find the same issues we should switch to 0.6.13 instead of 0.6.14.

@srbarrios
Copy link
Member

Since 4.3 FCS is coming soon, we can give this another try. I found dmacvicar/terraform-provider-libvirt#939 which seems a regression of 0.6.14. I have not tried it myself but if we find the same issues we should switch to 0.6.13 instead of 0.6.14.

After FCS 👍🏼

@nodeg
Copy link
Member

nodeg commented Jul 15, 2022

The SUMA kick off is nearly over. What about doing this next week?

@nodeg
Copy link
Member

nodeg commented Jul 20, 2022

@rjmateus Could you update SUSE/susemanager-ci#432 to be up to date with this one?

@rjmateus
Copy link
Member Author

@nodeg CI PR is updated. We can merge it. I would recommend do it on monday, so we can react on any problem.

@rjmateus rjmateus force-pushed the update_libvirt_0_6_12 branch from 780cec5 to f3a5370 Compare July 22, 2022 14:47
@nodeg
Copy link
Member

nodeg commented Jul 27, 2022

Thanks @rjmateus Sorry, I missed this notification until now. I just saw we need the updated package in systemsmanagement/sumaform in the OBS. There is still the old 0.6.3 version there. @juliogonzalez we should update the version there, too before merging this.

@nodeg
Copy link
Member

nodeg commented Jul 27, 2022

Ok so in our QE retro we discussed that we have to wait until the submissions of 4.3.1 before merging.

@srbarrios
Copy link
Member

Ok so in our QE retro we discussed that we have to wait until the submissions of 4.3.1 before merging.

Feel free to merge it now. We can revert if it cause any trouble.

@nodeg
Copy link
Member

nodeg commented Nov 30, 2022

We should consider updating directly to 0.7.0 which was released in October.

@nodeg nodeg changed the title update terraform libvirt provider to 0.6.14 Update terraform libvirt provider to 0.6.14 Nov 30, 2022
@nodeg nodeg changed the title Update terraform libvirt provider to 0.6.14 Update terraform-libvirt-provider Nov 30, 2022
@rjmateus
Copy link
Member Author

agree with @nodeg we should update to the latest. After merge we should solve the problems and not revert it

@nodeg
Copy link
Member

nodeg commented Dec 1, 2022

Thank you @rjmateus I agree, after merging we should solve potential issues and not just revert. If you need help, let me know.

@srbarrios
Copy link
Member

agree with @nodeg we should update to the latest. After merge we should solve the problems and not revert it

Sure. But please be sure you reserve time to fix it with urgency if it fails, avoid merging if you will not have time or you will be away.

@mcalmer
Copy link
Contributor

mcalmer commented Dec 1, 2022

But I would suggest first try to run this with the on-PR testsuite. You can change nearly everything in that job and point to branches of susemanager-ci, terraform, sumaform, etc. You should be able to give it a try there first.

@ktsamis
Copy link
Contributor

ktsamis commented Jan 10, 2023

Hey just a reminder that this needs some adjusting and testing again on PR testsuites. It would be the right time to merge these changes in the year otherwise we might delay it until next year again

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.

9 participants