Extended terraforming to create EFS terraform resource. #283

notjames · 2016-11-03T17:09:19Z

No description provided.

dtan4 · 2016-11-03T17:09:27Z

lib/terraforming/resource/efs.rb

+
+      def tfstate
+        efsystems.inject({}) do |resources, efs|
+          attributes = {


[rubocop]

Useless assignment to variable - attributes. :ref

dtan4 · 2016-11-03T17:09:27Z

lib/terraforming/resource/efs.rb

+      def tfstate
+        efsystems.inject({}) do |resources, efs|
+          attributes = {
+              "creationtoken" =>  efs.creationtoken,


[rubocop]

Operator => should be surrounded by a single space. :ref

coveralls · 2016-11-03T17:11:52Z

Coverage decreased (-0.4%) to 99.566% when pulling cedc5d5 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-03T17:17:55Z

lib/terraforming/resource/efs.rb

+      end
+
+      def tfstate
+        efsystems.inject({}){|resources, efs| resources}


[rubocop]

Space missing to the left of {.

Space between { and | missing.

Name inject block params |a, e|. :ref

Unused block argument - efs. If it's necessary, use _ or _efs as an argument name to indicate that it won't be used. :ref

Space missing inside }.

coveralls · 2016-11-03T17:20:02Z

Coverage decreased (-0.3%) to 99.662% when pulling 4d8fad3 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-03T17:20:03Z

Coverage decreased (-0.3%) to 99.662% when pulling 4d8fad3 on notjames:efs into bd8bb53 on dtan4:master.

…r vi swap files.

dtan4 · 2016-11-03T17:27:36Z

lib/terraforming/resource/efs.rb

+      end
+
+      def tfstate
+        efsystems.inject({}) { |resources, _efs| resources }


[rubocop]

Name inject block params |a, e|. :ref

Not really understanding this req. Checking into it more.

coveralls · 2016-11-03T17:29:56Z

Coverage decreased (-0.3%) to 99.662% when pulling 5f75427 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-03T17:29:56Z

Coverage decreased (-0.3%) to 99.662% when pulling 5f75427 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-03T17:42:21Z

Coverage decreased (-0.3%) to 99.71% when pulling 905ecdd on notjames:efs into bd8bb53 on dtan4:master.

…d rspec tests. Test work on local dev env.

dtan4 · 2016-11-04T03:34:48Z

lib/terraforming/resource/efs.rb

+      def tfstate
+        idx = -1
+
+        efsystems.inject({}) do  |resources, efs|


[rubocop]

Unnecessary spacing detected.

dtan4 · 2016-11-04T03:34:48Z

spec/lib/terraforming/resource/efs_spec.rb

+      end
+
+      let(:efs_description_0) do
+          {


[rubocop]

Use 2 (not 4) spaces for indentation. :ref

dtan4 · 2016-11-04T03:34:48Z

spec/lib/terraforming/resource/efs_spec.rb

+            number_of_mount_targets: 3,
+            owner_id: "999999999999",
+            performance_mode: "generalPurpose",
+            size_in_bytes: {value: 6144},


[rubocop]

Space inside { missing. :ref

Space inside } missing. :ref

dtan4 · 2016-11-04T03:34:48Z

spec/lib/terraforming/resource/efs_spec.rb

+      end
+
+      let(:efs_description_1) do
+          {


[rubocop]

Use 2 (not 4) spaces for indentation. :ref

dtan4 · 2016-11-04T03:34:49Z

spec/lib/terraforming/resource/efs_spec.rb

+            number_of_mount_targets: 3,
+            owner_id: "999999999999",
+            performance_mode: "generalPurpose",
+            size_in_bytes: {value: 23481234},


[rubocop]

Space inside { missing. :ref

Space inside } missing. :ref

dtan4 · 2016-11-04T03:34:49Z

spec/lib/terraforming/resource/efs_spec.rb

+      end
+
+      before do
+        client.stub_responses(:describe_file_systems, file_systems: [ efs_description_0, efs_description_1 ])


[rubocop]

Space inside square brackets detected. :ref

dtan4 · 2016-11-04T03:34:49Z

spec/lib/terraforming/resource/efs_spec.rb

+                    "deposed" => [],
+                    "provider" => "aws"
+                },
+                "aws_efs_file_system.efs.1" => {


[rubocop]

Align the elements of a hash literal if they span more than one line.

coveralls · 2016-11-04T03:36:58Z

Coverage decreased (-49.4%) to 50.562% when pulling 25b4f79 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-04T03:44:44Z

Coverage decreased (-49.4%) to 50.562% when pulling 1919cc0 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-04T03:45:46Z

spec/lib/terraforming/resource/efs_spec.rb

+                    "deposed"  => [],
+                    "provider" => "aws",
+                },
+                "aws_efs_file_system.efs.1" => {


[rubocop]

Align the elements of a hash literal if they span more than one line.

This style request is not making a great deal of sense to me with respect to this data object referenced.

coveralls · 2016-11-04T03:47:46Z

Coverage decreased (-49.4%) to 50.562% when pulling a5d6490 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-05T16:48:59Z

.gitignore

@@ -6,4 +6,5 @@
 /doc/
 /pkg/
 /spec/reports/
+*.sw?


Please do not include unnecessary change.

This was a necessary change for my environment since I use vim to edit code and I frequently suspend my editor while testing which leaves swap files open. If you prefer I still not add this then sobeit.

Press not that all fixes to code from this code review will be fine on Monday when I get back to work.

Thank you dtan!

Sorry. Autocorrect sucks.

"Press not" was meant to read please note. "Fine"should have been fixed.

If you're using vim, I strongly recommend to create ~/.gitignore and write the below.

[._]*.s[a-w][a-z] [._]s[a-w][a-z] # session Session.vim # temporary .netrwhist *~ # auto-generated tag files tags

(generated by https://www.gitignore.io/api/vim)

This gitignore affects globally in your machine. You do not have to write this settings at each repository.

dtan4 · 2016-11-05T17:11:37Z

lib/terraforming/resource/efs.rb

+        idx = -1
+
+        efsystems.inject({}) do |resources, efs|
+          idx += 1


How about this:

efsystems.each.with_index.inject({}) do |resources, (efs, idx)| # ... resources["aws_efs_file_system.efs.#{idx}"] = {

Awesome! This is much nicer.

dtan4 · 2016-11-05T17:12:45Z

lib/terraforming/resource/efs.rb

+
+      private
+
+      def efsystems


efsystems is not a good name. This should be file_systems.

with respect to using an unique identifier, I originally used the file_system_id for the name. I was unclear on how that would work for the array in the case of multiple FSes. I'll use this advice for now and hopefully there will be no issues with it. All cards on the table: I'm still not a heavy user of terraform; or in other words, I'm just getting started with it. So, yeah.

dtan4 · 2016-11-05T17:13:38Z

lib/terraforming/resource/efs.rb

+# Nov 2016
+module Terraforming
+  module Resource
+    class EFS


Please match Ruby class name to target Terraform resource name.
In this case, class name should be EFSFileSystem.

REF

https://www.terraform.io/docs/providers/aws/r/efs_file_system.html

dtan4 · 2016-11-05T17:14:06Z

lib/terraforming/resource/efs.rb

@@ -0,0 +1,62 @@
+# author: Jim Conner <snafu.x@gmail.com>
+# Nov 2016


Please do not include unnecessary comments.

dtan4 · 2016-11-05T17:17:52Z

Resource name must not contain period (.), so efs.#{idx} is invalid for resource name.

Errors:

  * aws_efs_file_system.efs.0: : invalid or unknown key: file_system_id
  * aws_efs_file_system.efs.0: efs.0: resource name can only contain letters, numbers, dashes, and underscores.

How about using other UNIQUE value, e.g. file_system_id?

dtan4 · 2016-11-05T17:18:19Z

Sorry for late review 🙇

notjames · 2016-11-06T06:34:19Z

No worries on late review. I'll fix these up and resubmit pull request.

dtan4 · 2016-11-07T19:05:24Z

lib/terraforming/resource/efs.rb

+      end
+
+      def tfstate
+        file_systems.each_with_index.inject({}) do |resources, (efs,idx)|


[rubocop]

Space missing after comma. :ref

Unused block argument - idx. If it's necessary, use _ or _idx as an argument name to indicate that it won't be used. :ref

dtan4 · 2016-11-07T19:05:25Z

lib/terraforming/resource/efs.rb

+            "tags.Name" => efs.name,
+          }
+
+          resources['%s' % [efs.file_system_id]] = {


[rubocop]

Favor format over String#%. :ref

coveralls · 2016-11-07T19:08:06Z

Coverage remained the same at 100.0% when pulling 96433f0 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-07T19:16:00Z

lib/terraforming/resource/efs.rb

+            "tags.Name" => efs.name,
+          }
+
+          resources[format('%s',efs.file_system_id)] = {


[rubocop]

Space missing after comma. :ref

…tate.

dtan4 · 2016-11-07T19:17:24Z

lib/terraforming/resource/efs.rb

+      end
+
+      def tfstate
+        file_systems.each_with_index.inject({}) do |resources, (efs, idx)|


[rubocop]

Unused block argument - idx. If it's necessary, use _ or _idx as an argument name to indicate that it won't be used. :ref

coveralls · 2016-11-07T19:18:23Z

Coverage remained the same at 100.0% when pulling c33d1f5 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-07T19:19:49Z

Coverage remained the same at 100.0% when pulling 5a80a4d on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-07T19:23:18Z

Coverage remained the same at 100.0% when pulling 45cb395 on notjames:efs into bd8bb53 on dtan4:master.

coveralls · 2016-11-07T22:52:49Z

Coverage remained the same at 100.0% when pulling 116fc47 on notjames:efs into bd8bb53 on dtan4:master.

notjames

All of these have been fixed.

dtan4 · 2016-11-11T00:41:47Z

spec/lib/terraforming/resource/efs_spec.rb

+      describe ".tfstate" do
+        it "should generate tfstate" do
+          expect(described_class.tfstate(client: client)).to eq({
+            "fs-0000abcd" => {


Is aws_efs-file-system. prefix really unnecessary? Other resources has resource name prefix; e.g. https://github.com/notjames/terraforming/blob/master/spec/lib/terraforming/resource/ec2_spec.rb#L455

see previous note. This is fixed now.

dtan4 · 2016-11-11T00:43:30Z

lib/terraforming/resource/efs.rb

+            "tags.Name" => efs.name,
+          }
+
+          resources[format('%s', efs.file_system_id)] = {


Please use string interpolation to inject a certain value in String literal.

Actually in this case, it seems that neither string interpolation or format is required...

Looks like I did in fact miss the aws_efs_file_system prefix in the resource name. Also changed to interpolation (a style that I personally hate, btw, but you're the owner, so... :))

coveralls · 2016-11-11T17:19:18Z

Coverage remained the same at 100.0% when pulling aa50486 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-13T14:42:33Z

One last thing... please rename efs.rb and efs_spec.rb to efs_file_system.rb and efs_file_system_spec.rb. Mostly in Ruby, class name and file name should be matched.

notjames · 2016-11-13T20:48:42Z

Will do it tonight and push the change.

… Ruby class names.

coveralls · 2016-11-14T10:58:32Z

Coverage remained the same at 100.0% when pulling d3183b9 on notjames:efs into bd8bb53 on dtan4:master.

dtan4 · 2016-11-16T15:37:00Z

LGTM 👍 Thanks ❗️

Extended terraforming to create EFS terraform resource.

cedc5d5

dtan4 reviewed Nov 3, 2016

View reviewed changes

Updated to clear CI tests. Polished resources injection.

4d8fad3

dtan4 reviewed Nov 3, 2016

View reviewed changes

Polished more per style guide (per last pull req). Added gitignore fo…

5f75427

…r vi swap files.

dtan4 reviewed Nov 3, 2016

View reviewed changes

removed tfstate method. Seems it is not necessary (for now)

905ecdd

Jim Conner added 2 commits November 3, 2016 12:18

Added efs to cli_spec so tests will pass.

c345185

Added tf_state method back in...need it after all. Who knew? And adde…

25b4f79

…d rspec tests. Test work on local dev env.

dtan4 reviewed Nov 4, 2016

View reviewed changes

spec/lib/terraforming/resource/efs_spec.rb

end

let(:efs_description_0) do

{

Copy link

Owner

dtan4 Nov 4, 2016

Choose a reason for hiding this comment

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

[rubocop]

Use 2 (not 4) spaces for indentation. :ref

dtan4 reviewed Nov 4, 2016

View reviewed changes

spec/lib/terraforming/resource/efs_spec.rb

end

let(:efs_description_1) do

{

Copy link

Owner

dtan4 Nov 4, 2016

Choose a reason for hiding this comment

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

[rubocop]

Use 2 (not 4) spaces for indentation. :ref

dtan4 reviewed Nov 4, 2016

View reviewed changes

Fixed style-guide fails during rubocop CI check for pull-request.

1919cc0

dtan4 reviewed Nov 4, 2016

View reviewed changes

removed require for awesome_print gem.

a5d6490

more style violation fixes

740dbac

dtan4 suggested changes Nov 5, 2016

View reviewed changes

Changes made to respect code review dtan4#283 (review)

96433f0

dtan4 reviewed Nov 7, 2016

View reviewed changes

Style change per code review for string formatting of resource in tfs…

c33d1f5

…tate.

dtan4 reviewed Nov 7, 2016

View reviewed changes

Style change per code review for missing space after comma

5a80a4d

Removed useless use of each_with_index.

45cb395

add comma in format() for second argument.

116fc47

notjames commented Nov 9, 2016

View reviewed changes

dtan4 reviewed Nov 11, 2016

View reviewed changes

dtan4 suggested changes Nov 11, 2016

View reviewed changes

Fixed reference to resource in tfstate in both lib and spec files.

aa50486

style change and request by code maintainer to mv file names to match…

d3183b9

… Ruby class names.

dtan4 approved these changes Nov 16, 2016

View reviewed changes

dtan4 merged commit d78824c into dtan4:master Nov 16, 2016

		@@ -0,0 +1,62 @@
		# author: Jim Conner <snafu.x@gmail.com>
		# Nov 2016

Extended terraforming to create EFS terraform resource. #283

Extended terraforming to create EFS terraform resource. #283

Conversation

notjames commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 3, 2016

coveralls commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 3, 2016 • edited Loading

coveralls commented Nov 3, 2016

coveralls commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 4, 2016 • edited Loading

coveralls commented Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

REF

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtan4 commented Nov 5, 2016

dtan4 commented Nov 5, 2016

notjames commented Nov 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 7, 2016 • edited Loading

coveralls commented Nov 7, 2016 • edited Loading

coveralls commented Nov 7, 2016 • edited Loading

coveralls commented Nov 7, 2016 • edited Loading

notjames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 11, 2016 • edited Loading

dtan4 commented Nov 13, 2016

notjames commented Nov 13, 2016

coveralls commented Nov 14, 2016 • edited Loading

dtan4 commented Nov 16, 2016

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 3, 2016 •

edited

Loading

coveralls commented Nov 4, 2016 •

edited

Loading

coveralls commented Nov 4, 2016 •

edited

Loading

coveralls commented Nov 4, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading

coveralls commented Nov 7, 2016 •

edited

Loading

coveralls commented Nov 11, 2016 •

edited

Loading

coveralls commented Nov 14, 2016 •

edited

Loading