[Feature] - Pass tags through a tfvars vs cli #431

codezninja · 2022-03-30T02:18:58Z

This PR will support the ability to pass tags through a generated tfvars file instead. This simplifies the cli command.

This is beneficial for custom plugins that might need wrap the full terraform commands with some other commands for example su ec2-user -c "{generate terraform}" without worrying about all the escaping of multiple quotes

Fixes #432

Merge Checklist

Have you linked this Pull Request to an Issue?
Have you updated any relevant README files, to explain how this new feature works (with examples)?
Have you added relevant test cases for your change?
Do all tests and code style checks pass with ./gradlew check --info?
Have you successfully run your change in a pipeline in a Jenkins instance?
Have you updated the CHANGELOG?

codecov-commenter · 2022-03-30T02:20:13Z

Codecov Report

Merging #431 (ea20c95) into master (d1a5e98) will decrease coverage by 0.66%.
The diff coverage is 7.69%.

@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
- Coverage   82.44%   81.78%   -0.67%     
==========================================
  Files          54       54              
  Lines        1487     1499      +12     
  Branches      316      319       +3     
==========================================
  Hits         1226     1226              
- Misses        115      126      +11     
- Partials      146      147       +1

Impacted Files	Coverage Δ
src/Jenkinsfile.groovy	`75.38% <0.00%> (-2.40%)`	⬇️
src/TerraformPlanCommand.groovy	`85.93% <0.00%> (-8.90%)`	⬇️
src/TagPlugin.groovy	`81.81% <20.00%> (-8.19%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a5e98...ea20c95. Read the comment docs.

codezninja · 2022-04-01T00:13:35Z

@kmanning Let us know if the pr need to be updated.

kmanning · 2022-04-04T15:52:03Z

docs/TagPlugin.md

        .build()
 ```

+If you want to simplify the cli command you can pass the tags using `-var-file` instaed.


Type: 'instaed'

Can this be clarified?
If you want to simplify the cli command, you can use the .writeToFile() method to write tags out to a file and pass them using -var-file instead

Type: 'instaed'

corrected

kmanning · 2022-04-04T15:53:42Z

docs/TagPlugin.md

+
+def validate = new TerraformValidateStage()
+
+// Tag variables will be passsed via `-var-file`


typo passsed

kmanning · 2022-04-04T15:56:25Z

src/TerraformPlanCommand.groovy

+        String value = convertMapToCliString(values)
+        Jenkinsfile.writeFile(fileName, "${key}=${value}")
+        return withVariableFile(fileName)
+    }


In a future refactoring of this feature, I could imagine pushing the writing of the file up to the TagPlugin. That could reduce duplication a bit.

kmanning · 2022-04-04T15:56:49Z

src/TerraformApplyCommand.groovy

+        String value = convertMapToCliString(values)
+        Jenkinsfile.writeFile(fileName, "${key}=${value}")
+        return withVariableFile(fileName)
+    }


In a future refactoring of this feature, I could imagine pushing the writing of the file up into TagPlugin. It would reduce duplication a bit.

kmanning · 2022-04-04T15:57:15Z

src/TagPlugin.groovy

+    public static writeToFile() {
+        writeToFile = true
+        return this
+    }


In a future refactoring of this feature, I could imagine writeToFile accepting an option string, to overwrite the file name that is written. The default value of the optional string parameter could be the filename that you're currently using <environment>-tags.tfvars

kmanning · 2022-04-04T16:02:26Z

test/TagPluginTest.groovy

+
+            verify(command, times(1)).withVariable(anyString(), anyMap())
+            verifyNoMoreInteractions(command)
+        }


This is fine as it is, but in this test, you're actually testing apply and not writeToFile. You could optionally group this under ApplyForApplyCommand.

I would also consider that this specific test case is already covered by

terraform-pipeline/test/TagPluginTest.groovy

Line 82 in b8e9c79

public void addsTheTagArgument() {

Understood. I guess I was just verifying the behavior is changed between "normally" and "writesToFile". I will just remove this one.

kmanning · 2022-04-04T16:04:37Z

test/TagPluginTest.groovy

+            verify(command, times(1)).withVariableFile(anyString(), anyMap())
+            verifyNoMoreInteractions(command)
+        }
+    }


This is fine as it is, but in this test, you're actually testing apply and not writeToFile. You could optionally group this under ApplyForApplyCommand.

Further, you're testing a specific condition of apply - the condition = when writeToFile is used. I might consider nesting that, similar to what is happening with WithVariableName

True. I'll move it

kmanning · 2022-04-04T16:07:08Z

test/TerraformApplyCommandTest.groovy

+            verify(original).writeFile(file: filename.toString(), text: content.toString())
+            verifyNoMoreInteractions(original)
+        }
+    }


This is fine, but I think this test case wouldn't exist, if writing-the-file behavior lived in TagPlugin.

kmanning · 2022-04-04T16:07:35Z

test/TerraformPlanCommandTest.groovy

+            def content = "${myKey}={expectedKey=\"${expectedValue}\"}"
+            verify(original).writeFile(file: filename.toString(), text: content.toString())
+            verifyNoMoreInteractions(original)
+        }


This is fine, but I think this test case wouldn't exist as-is, if writing-the-file behavior lived in TagPlugin.

kmanning · 2022-04-04T16:16:08Z

test/TagPluginTest.groovy

+            verifyNoMoreInteractions(command)
+        }
+    }
 }


You've tested TagPlugin with TerraformApplyCommand, but not TerraformPlanCommand? Gross, that's because the interface for apply is not very clean... Disregard this comment.

kmanning

kmanning · 2022-04-18T17:21:41Z

Please squash and merge this request when ready.

codezninja added 24 commits March 29, 2022 11:04

testing support for passing tags through variable file

280e8cd

remove extra single-quote

c125281

debugging issue

9eeda09

lets create a file and see what happens

e3f6503

lets append to file

60f625a

maybe its the function call

412d25f

let us just append a simple string

1eaf686

reference this for environment variable

a59cc3b

all simple characters

c98c8b8

lets try calling create file and logging it

ac17646

resetting testing env

fada48f

maybe we need workspace

b27d6ad

let see if workspace comes to plan cli

3272aaf

let try creating the file again

596a47c

let use extra variable type for File

20e459d

lets see if workspace exists

ff6c1b6

let us use FilePath instead

8d75f69

import FilePath

35dfbb7

now I am just shooting blind

909bacf

let use Jenkinsfile.instance to write content

bb84f89

now we are getting somewhere

5186602

no need to create a custom writeFile function

870dcd9

allow tags passed through via file a choice

8dd72ea

revert: no need to create a custom writeFile function

ea20c95

codezninja self-assigned this Mar 30, 2022

codezninja added 3 commits March 29, 2022 22:38

reorder functions in tags

6051e84

simple test for writeToFile check

2746aea

add variableFile fucntion to TerraformApply

9adceda

codezninja force-pushed the feature/pass_tags_through_file branch from f5c5658 to 9adceda Compare March 30, 2022 02:44

updated changelog and readme

9d04027

codezninja marked this pull request as ready for review March 30, 2022 02:51

codezninja requested a review from kmanning March 30, 2022 02:52

treylittle and others added 3 commits March 31, 2022 11:34

add tests for TagPlugin and Jenkinsfile

3c9acc6

fix ordering to follow pattern

d73b38d

add tests for TerraformApplyCommand and TerraformPlanCommand

b8e9c79

kmanning reviewed Apr 12, 2022

View reviewed changes

update's from Keith's review comments

dac28b9

kmanning approved these changes Apr 18, 2022

View reviewed changes

codezninja merged commit b4cec15 into master Apr 18, 2022

codezninja deleted the feature/pass_tags_through_file branch April 18, 2022 18:26

codezninja restored the feature/pass_tags_through_file branch August 15, 2022 16:29


		def validate = new TerraformValidateStage()

		// Tag variables will be passsed via `-var-file`

[Feature] - Pass tags through a tfvars vs cli #431

[Feature] - Pass tags through a tfvars vs cli #431

Uh oh!

Conversation

codezninja commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

Uh oh!

codecov-commenter commented Mar 30, 2022

Codecov Report

Uh oh!

codezninja commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmanning Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

treylittle Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmanning left a comment

Choose a reason for hiding this comment

Uh oh!

kmanning commented Apr 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codezninja commented Mar 30, 2022 •

edited

Loading

codezninja commented Apr 1, 2022 •

edited

Loading

kmanning Apr 4, 2022 •

edited

Loading

treylittle Apr 14, 2022 •

edited

Loading