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

terraform module: Added backend_config_file and multiple variables_file #394

Merged
merged 11 commits into from Jun 3, 2020

Conversation

m-yosefpor
Copy link
Contributor

SUMMARY

This PR adds support for multiple variables_file for "terraform" module, Fixes #224 issue. Moreover, this adds support for "backend_config_file" to provide at init state to the -backend-config parameter. This can accept a list of paths to multiple configuration files.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

terraform module

ADDITIONAL INFORMATION

It is possible to pass multiple var-files in terraform cli:

terraform apply -var-file="file-1.tfvars" -var-file="file-2.tfvars" -var-file="file-3.tfvars"

This is required when there are multiple var-files in different locations.

Also the "-backend-config" parameter, accepts both "key=value" type and "PATH" type. This parameter can also be passed multiple backend config files:

terraform init -backend-config=path/to/backend_config_file_1 -backend-config=path/to/backend_config_file_2

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor labels May 21, 2020
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

plugins/modules/cloud/misc/terraform.py:287:43: bad-whitespace: Exactly one space required after comma             variables_file=dict(type='list',default=None),                                            ^
plugins/modules/cloud/misc/terraform.py:320:61: bad-whitespace: Exactly one space required after comma         init_plugins(command[0], project_path, backend_config,backend_config_file)                                                              ^

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

plugins/modules/cloud/misc/terraform.py:100:161: E501: line too long (165 > 160 characters)
plugins/modules/cloud/misc/terraform.py:287:44: E231: missing whitespace after ','
plugins/modules/cloud/misc/terraform.py:320:62: E231: missing whitespace after ','
plugins/modules/cloud/misc/terraform.py:342:11: E111: indentation is not a multiple of four

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label May 21, 2020
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label May 21, 2020
@m-yosefpor
Copy link
Contributor Author

m-yosefpor commented May 21, 2020

zuul fails with:

'ERROR! module community.general.intersight_info missing documentation

hmm.. so what should I do? It has nothing to do with my change! I think it is probably related to this latest commit : 7bb36d6

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

You need a changelog fragment.

plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

You can ignore the failing Zuul tests for now.

m-yosefpor and others added 3 commits May 22, 2020 12:55
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
specify type

Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/cloud/misc/terraform.py:0:0: doc-elements-mismatch: Argument 'variables_file' in argument_spec specifies elements as path,but elements is not documented

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label May 22, 2020
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label May 22, 2020
Copy link
Collaborator

@felixfontein felixfontein 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 to me. I don't know terraform though, so no idea whether this change works as expected and makes sense.

@m-yosefpor
Copy link
Contributor Author

File: A configuration file may be specified via the init command line. To specify a file, use the -backend-config=PATH option when running terraform init.

If backend settings are provided in multiple locations, the top-level settings are merged such that any command-line options override the settings in the main configuration and then the command-line options are processed in order, with later options overriding values set by earlier options.

https://www.terraform.io/docs/backends/config.html#partial-configuration

I have actually tested the module locally.

BTW, Thanks Felix, you helped a lot. I've got familiar with how to create appropriate PRs for ansible modules (changelogs, sanity checks, etc). Future contributions are on the way :)

@m-yosefpor
Copy link
Contributor Author

ERROR! module community.general.kubevirt_pvc missing documentation

@felixfontein
Copy link
Collaborator

That's the error in Zuul. The problem is that in the Zuul tests, the collection dependencies of community.general are not installed. Simply ignore the Zuul tests for now, right now they are not helpful at all. (On Shippable the same tests runs fine, because the dependencies are installed.)

@m-yosefpor
Copy link
Contributor Author

Aha. Ok, so what is the next step? Do I need to take extra steps for this PR to be merged?

@felixfontein
Copy link
Collaborator

@m-yosefpor you need to find someone to review/test this who actually uses terraform. Maybe look at issues/PRs for terraform in this repo, there seem to be other people also interested in this module.

@m-yosefpor
Copy link
Contributor Author

@Andersson007 I wonder if you are the one person to review this PR. You are the only one who has committed regarding terraform.py. (and an infra engineer). Thank you in advanced.

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@m-yosefpor hey, unfortunately i don't use terraform. We need somebody who can try this without learning the tool from scratch

changelogs/fragments/394-terraform-add-config_file.yml Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

CC @ryansb as the module's author could you please look at the changes? I'll text you by mail as well

@felixfontein
Copy link
Collaborator

@Andersson007 Ryan used to work for Ansible, and switched jobs some time ago, so I'm not sure whether he's still interested. Especially since he is in the ignore BOTMETA list for cloud/misc/.

@felixfontein
Copy link
Collaborator

@effaamponsah @FrancisBilla you both created a PRs for the terraform module recently, so apparently you're also interested in it.

@Andersson007
Copy link
Contributor

@felixfontein ah, thanks for the info

Added PR URL.

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@FrancisBilla
Copy link

@effaamponsah @FrancisBilla you both created a PRs for the terraform module recently, so apparently you're also interested in it.

yeah!!!

@Andersson007
Copy link
Contributor

@FrancisBilla if the changes are ok for you, please, approve the PR and we'll merge it

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 3, 2020
@m-yosefpor
Copy link
Contributor Author

Does stale_ci label need any action from me?

@Andersson007
Copy link
Contributor

Does stale_ci label need any action from me?

no

@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jun 3, 2020
@Andersson007
Copy link
Contributor

i've just relaunched the tests to get rid of the label

@FrancisBilla
Copy link

@FrancisBilla if the changes are ok for you, please, approve the PR and we'll merge it

Looks good for me, @felixfontein and his team are in charge

@Andersson007 Andersson007 merged commit 116978a into ansible-collections:master Jun 3, 2020
@Andersson007
Copy link
Contributor

@m-yosefpor thanks for the contribution!
@felixfontein @FrancisBilla thanks for reviewing!

@Andersson007
Copy link
Contributor

merged #394 into master

@m-yosefpor
Copy link
Contributor Author

m-yosefpor commented Jun 3, 2020

@m-yosefpor thanks for the contribution!
@felixfontein @FrancisBilla thanks for reviewing!

thank you too, for your welcoming community.. more one the way ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terraform: Add support for multiple variables_file
5 participants