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

WIP: Import changes from gitlab/dda/python-terraform #123

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Th0masL
Copy link

@Th0masL Th0masL commented Jul 17, 2022

This PR contains the changes to support Terraform version 1.x, that have been applied on the gitlab repository dda/python-terraform

@Th0masL Th0masL marked this pull request as draft July 17, 2022 07:13
@Th0masL Th0masL changed the title Import changes from gitlab/dda/python-terraform WIP: Import changes from gitlab/dda/python-terraform Jul 17, 2022
@Th0masL Th0masL marked this pull request as ready for review July 17, 2022 07:14
@@ -52,6 +52,7 @@ def __init__(
var_file: Optional[str] = None,
terraform_bin_path: Optional[str] = None,
is_env_vars_included: bool = True,
terraform_version: Optional[float] = 0.13
Copy link

Choose a reason for hiding this comment

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

Maybe we can find here sth better than float to encode terraform versions?
We will need comparison operators on the encoded versions.
Any idea?

Copy link
Author

Choose a reason for hiding this comment

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

What is the purpose of this value ? Can't we detect the version that is currently installed and use it ?

Copy link

Choose a reason for hiding this comment

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

I've added a second comment at a location where we use the terraform version. There are many more locations like the commented one.

Copy link
Author

@Th0masL Th0masL Jul 19, 2022

Choose a reason for hiding this comment

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

We could eventually encode the Terraform Versions values as string, and use packaging (more examples here) to compare the values ?

>>> from packaging import version
>>> version.parse("2.3.1") < version.parse("10.1.2")
True

Copy link

Choose a reason for hiding this comment

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

sounds good :-)

Copy link

Choose a reason for hiding this comment

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

Implemented your suggestion on our repo:
https://gitlab.com/domaindrivenarchitecture/python-terraform/-/merge_requests/2

What do you think ?

If you agree I will sqash & merge. You can bring back my change by cherry pick?

@jerger
Copy link

jerger commented Jul 18, 2022

I think, we should reflect all changes since june 21 in the PR ...

@Th0masL
Copy link
Author

Th0masL commented Jul 18, 2022

Just to make sure I understand, you think we need to take any changes that has been applied on this repo since last time your forked it on gitlab, and re-apply them on top of your changes ?

I've re-applied the changes since June 2021.

Let me know if you see any other thing missing

default = kwargs.copy()
default["force"] = force
# force is no longer a flag in version >= 1.0
if self.terraform_version < 1.0:
Copy link

Choose a reason for hiding this comment

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

Here we use the terraform_version in order to distinguishe between 0.x & 1.x

Copy link
Author

Choose a reason for hiding this comment

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

I see 👍

@jerger
Copy link

jerger commented Aug 5, 2022

Reintegrated the upstream changes & got green tests


def list_workspace(self) -> List[str]:
"""List of workspaces

Copy link

Choose a reason for hiding this comment

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

global_opts = self._generate_default_general_options(False) is needed here.


def list_workspace(self) -> List[str]:
"""List of workspaces

Copy link

Choose a reason for hiding this comment

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

Line 454 should be (self.cmd(global_opts, "workspace", "list")[1] or '').split()

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.

2 participants