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

make sumaform terraform v11 compatible #387

Merged
merged 14 commits into from
Aug 28, 2018

Conversation

MalloZup
Copy link
Contributor

@MalloZup MalloZup commented Jul 24, 2018

This pr is not taking care of the update to v11 , it is a prerequisite step that we need to implement.

fix #285

Left to do:

gitlab

https://gitlab.suse.de/galaxy/sumaform-test-runner/merge_requests/41

backup centos7 or sles img

communication:

  • send an-email to galaxy-devel about the change of API (version != product_version)

@MalloZup MalloZup requested a review from moio July 24, 2018 10:28
Copy link
Contributor

@moio moio left a comment

Choose a reason for hiding this comment

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

  • see inline comments
  • modules/libvirt/pts/minion-dump.mp and modules/openstack/pts/minion-dump.mp binary files should not change

@@ -1,10 +1,10 @@
# Advanced `main.tf` configurations

## Changing product versions
## Changing product version
Copy link
Contributor

Choose a reason for hiding this comment

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

versions was OK here

@@ -191,11 +191,11 @@ module "min" {
}
```

## Change the base OS for supported SUSE Manager versions
## Change the base OS for supported SUSE Manager product_versions
Copy link
Contributor

Choose a reason for hiding this comment

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

versions was OK here


You can specifiy a base OS for `suse_manager` modules by specifying an `image` variable. There is a default selection if nothing is specified. Please note that not all SUSE Manager versions support all OSs, refer to official documentation to select a compatible OS. In particular, `opensuse423` can presently only be used for the `head` version.
You can specifiy a base OS for `suse_manager` modules by specifying an `image` variable. There is a default selection if nothing is specified. Please note that not all SUSE Manager product_versions support all OSs, refer to official documentation to select a compatible OS. In particular, `opensuse423` can presently only be used for the `head` product_version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please double check all documentation or other non-code places. product_versions should be only changed in code, examples, and explicit references to code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yop

variable "version" {
description = "A valid SUSE Manager version (eg. 3.0-nightly, head) see README_ADVANCED.md"
variable "product_version" {
description = "A valid SUSE Manager product_version (eg. 3.0-nightly, head) see README_ADVANCED.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

version was correct here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@moio ok i can check the stuff, is really not easy to check all the stuff manually 🤕

@MalloZup MalloZup requested a review from mcalmer July 25, 2018 13:11
@MalloZup
Copy link
Contributor Author

MalloZup commented Jul 25, 2018

Copy link
Contributor

@moio moio left a comment

Choose a reason for hiding this comment

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

Almost there:

  • one last inline comment
  • please update required_version = ">= 0.10.7" blocks throughout the codebase to request Terraform 0.11.x (we want users to upgrade to that version and we are already pretty certain 0.12 will break sumaform at this point)

Some modules have a ``version`` variable that determines the software version. Specifically:
* in `suse_manager`, `suse_manager_proxy` etc. `version` determines the SUSE Manager product version,
* in `minion`, `client`, etc. `version` determines the SUSE Manager Tools version.
Some modules have a ``product_version`` variable that determines the software product version. Specifically:
Copy link
Contributor

Choose a reason for hiding this comment

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

Double backticks here

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.

I could not find any problem, but I'd strongly suggest using grep with the branch used for the PR, to see that we are not missing anything :-)

Copy link
Contributor

@moio moio left a comment

Choose a reason for hiding this comment

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

Two more corner cases caught.

Apart from those this PR could be merged but we can't do that before having a terraform package that builds on Leap 15. Just too many people are on that distro.

I am fine with a patch that disables that test.

@@ -1,5 +1,5 @@
provider "openstack" {
version = "~> 1.2"
product_version = "~> 1.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong, as version is an internal variable of the provider in this case.

@@ -1,5 +1,5 @@
provider "openstack" {
version = "~> 1.2"
product_version = "~> 1.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

LGTM

@MalloZup MalloZup changed the title WIP: make sumaform terraform v11 compatible make sumaform terraform v11 compatible Aug 23, 2018
in bridged mode we need it, so is better to use it always
@uyuni-project uyuni-project deleted a comment from moio Aug 28, 2018
@uyuni-project uyuni-project deleted a comment from juliogonzalez Aug 28, 2018
@uyuni-project uyuni-project deleted a comment from moio Aug 28, 2018
@uyuni-project uyuni-project deleted a comment from juliogonzalez Aug 28, 2018
@MalloZup MalloZup merged commit b4673ec into uyuni-project:master Aug 28, 2018
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.

Support terraform v0.11
4 participants