Fix VPC parser #389

ddiachkov · 2017-09-11T12:21:16Z

This PR fixes problems with VPC parser + adds cidr_block_association_set attribute to model.
#379 was not sufficient – VPC list still contains duplicates with invalid status.

NB: cidr_block_association_set and ipv_6_cidr_block_association_set are array of hashes (currently ipv_6_cidr_block_association_set is a Hash)

geemus

Overall looking good, just added some minor feedback. Thanks!

geemus · 2017-09-12T20:11:05Z

lib/fog/aws/models/compute/vpc.rb

+
+        attribute :cidr_block_association_set, :aliases => 'cidrBlockAssociationSet'
+
+        attribute :ipv_6_cidr_block_association_set, :aliases => 'ipv6CidrBlockAssociationSet'


I guess it was already there, but it seems surprising to me that this is ipv_6_cidr_block_association_set instead of ipv6_cidr_block_association_set. Does that make sense? I just happened to notice here (and have the same question about the new attribute below). What do you think?

I think you are right. We could just add alias for backward compatibility (ipv_6_cidr_block_association_set is already released...)

I hadn't even thought about that, but that sounds good. That way we can have more consistent names, but avoid breaking anybody. Sound good?

geemus · 2017-09-12T20:16:07Z

lib/fog/aws/parsers/compute/describe_vpcs.rb

+                @current_cidr_block[name] = value
+
+              when 'vpcSet.item.cidrBlockAssociationSet.item.cidrBlockState'
+                @current_cidr_block['state'] = value.squish


In the old version state was capitalized as State, not sure it makes that much difference, but probably better safe than sorry. Could you change it?

I don't like the inconsistency:

{ "cidrBlock" => "...", "associationId" => "...", "State" => "associated" }

Can we sacrifice backward compatibility in that case (knowing that ipv_6_cidr_block_association_set support just released)?

I don't feel super strongly about it, so I'm alright leaving it as you suggest. I just wanted to at least discuss it, which we now have. Thanks!

geemus · 2017-09-12T20:16:24Z

lib/fog/aws/parsers/compute/describe_vpcs.rb

+                @current_ipv6_block[name] = value
+
+              when 'vpcSet.item.ipv6CidrBlockAssociationSet.item.ipv6CidrBlockState'
+                @current_ipv6_block['state'] = value.squish


Same thing here, if we could keep it as State for safety that would be great.

geemus · 2017-09-12T20:17:42Z

@lanej This looks good to me other than my feedback, but I would appreciate an extra set of eyes if you have a chance. Thanks!

lanej · 2017-09-25T18:10:47Z

Seeing some failure when doing live testing

irb(main):003:0> aws.describe_vpcs
#<NoMethodError: undefined method `squish' for "associated\n                    ":String>
#<NoMethodError: undefined method `squish' for "associated\n                    \n                ":String>
#<NoMethodError: undefined method `squish' for #<String:0x007fdf881e4898>>
#<NoMethodError: undefined method `squish' for "false\n        ":String>
#<NoMethodError: undefined method `squish' for "true\n        \n    ":String>
#<NoMethodError: undefined method `squish' for "true\n        \n    \n":String>

String#squish is an rails-ism that we do not have access to.

geemus · 2017-09-25T20:49:41Z

I'm fine with this going in and adding the aliases later too, if that is easier.

lanej · 2017-09-25T20:54:10Z

A squish reference was mistakenly introduced in 9e97b41.

@ddiachkov please rebase this PR and resolve conflicts, when you're ready.

ddiachkov · 2017-10-01T08:28:35Z

@lanej done

lanej · 2017-10-05T19:25:33Z

Live tests result:

» FOG_MOCK=false be shindo tests/requests/compute/vpc_tests.rb
[fog][WARNING] Setting default fog timeout to 2000 seconds

  Fog::Compute[:aws] | vpc requests (aws)
    success
      #create_vpc + has proper format
      #create_vpc + has proper format
      #describe_vpcs - has proper format
        {"vpcSet"=>[{"tagSet"=>{}, "ipv6CidrBlockAssociationSet"=>{}, "vpcId"=>"vpc-9ee916e6", "state"=>"associated", "cidrBlock"=>"10.255.254.0/28"}, {"tagSet"=>{}, "ipv6CidrBlockAssociationSet"=>{}, "dhcpOptionsId"=>"dopt-d04fd7b4", "instanceTenancy"=>"default", "isDefault"=>false}, {"tagSet"=>{}, "ipv6CidrBlockAssociationSet"=>{}, "vpcId"=>"vpc-44ed123c", "state"=>"associated", "cidrBlock"=>"10.255.254.0/28"}, {"tagSet"=>{}, "ipv6CidrBlockAssociationSet"=>{}, "dhcpOptionsId"=>"dopt-d04fd7b4", "instanceTenancy"=>"default", "isDefault"=>false}], "requestId"=>"9a9e14cd-ccd0-4a07-bcd5-674e5a34de41"} does not match {"vpcSet"=>[{"vpcId"=>String, "state"=>String, "cidrBlock"=>String, "dhcpOptionsId"=>String, "tagSet"=>Hash, "instanceTenancy"=>Fog::Nullable::String}], "requestId"=>String}

Beyond that, I think this is ready to go.

lanej · 2017-10-05T19:26:59Z

@geemus I am still getting nil VPC ids with this change :(.

ddiachkov · 2017-10-05T20:50:18Z

@lanej thats not right... Which field contains nil?

lanej · 2017-10-05T20:59:59Z

<Fog::Compute::AWS::VPC
        id=nil,
        state=nil,
        cidr_block=nil,
        dhcp_options_id="dopt-d04fd7b4",
        tags={},
        tenancy="default",
        is_default=false,
        ipv_6_cidr_block_association_set={},
        amazon_provided_ipv_6_cidr_block=false
      >

@ddiachkov seems the id is still nil, as reported in #387

ddiachkov · 2017-10-05T21:00:43Z

Let me check it on a real AWS

ddiachkov · 2017-10-06T06:48:02Z

Hey @lanej, are you sure you use this branch?

  [{"tagSet"=>{},
    "cidrBlockAssociationSet"=>
     [{"cidrBlock"=>"10.255.254.0/28",
       "associationId"=>"vpc-cidr-assoc-687a8b02",
       "state"=>"associated"}],
    "ipv6CidrBlockAssociationSet"=>[],
    "vpcId"=>"vpc-cab34db2",
    "state"=>"available",
    "cidrBlock"=>"10.255.254.0/28",
    "dhcpOptionsId"=>"dopt-2873c74c",
    "instanceTenancy"=>"default",
    "isDefault"=>false},
   {"tagSet"=>{},
    "cidrBlockAssociationSet"=>
     [{"cidrBlock"=>"172.31.0.0/16",
       "associationId"=>"vpc-cidr-assoc-473c8e2e",
       "state"=>"associated"}],
    "ipv6CidrBlockAssociationSet"=>[],
    "vpcId"=>"vpc-3e9d1659",
    "state"=>"available",
    "cidrBlock"=>"172.31.0.0/16",
    "dhcpOptionsId"=>"dopt-2873c74c",
    "instanceTenancy"=>"default",
    "isDefault"=>true},
   {"tagSet"=>{},
    "cidrBlockAssociationSet"=>
     [{"cidrBlock"=>"10.255.254.0/28",
       "associationId"=>"vpc-cidr-assoc-c17988ab",
       "state"=>"associated"}],
    "ipv6CidrBlockAssociationSet"=>[],
    "vpcId"=>"vpc-5cb04e24",
    "state"=>"available",
    "cidrBlock"=>"10.255.254.0/28",
    "dhcpOptionsId"=>"dopt-2873c74c",
    "instanceTenancy"=>"default",
    "isDefault"=>false}],
 "requestId"=>"920616e6-36fd-4779-9c41-21f0518aa52c"}

Thats what I get on a real AWS. Notice that ipv_6_cidr_block_association_set is an Array, not Hash.

ddiachkov · 2017-10-06T06:48:50Z

I've added additional format tests.

lanej · 2017-10-06T16:44:08Z

Thanks @ddiachkov.

Hey @geemus, how would you feel about moving the Gemfile.edge tests to allowed failures in Travis CI? This branch is failing due to an unrelated change on fog-core, I think fog/fog-core#218

geemus · 2017-10-17T17:04:45Z

@lanej feel free to do that if you think it makes sense (though presumably we'll want to also fix that before release?).

lanej · 2017-10-17T20:23:39Z

@ddiachkov sorry for the delay. If you rebase off master, we can get this out.

ddiachkov · 2017-10-17T22:30:06Z

No worries. I've rebased the code – looks like tests are passed.

lanej · 2017-10-17T22:30:56Z

@geemus has your feedback been addressed?

geemus · 2017-10-18T14:46:24Z

@lanej I think so, thanks for double checking!

geemus · 2017-10-18T14:46:45Z

@ddiachkov thanks!

geemus requested changes Sep 12, 2017

View reviewed changes

chanakyacool mentioned this pull request Sep 19, 2017

Fog::Compute::AWS::Vpcs returns VPCs with nil ids #387

Closed

ddiachkov force-pushed the fix_vpc_parser branch 4 times, most recently from f8aa51b to 3a44e60 Compare October 1, 2017 07:58

ddiachkov force-pushed the fix_vpc_parser branch from f8aa51b to a4fe5de Compare October 6, 2017 02:18

ddiachkov added 6 commits October 17, 2017 12:11

fix and refactor VPC parsers

17df196

replace unholy squish with strip

593886b

rename ipv_6 to ipv6 in VPC attributes (with backward compatibility)

e7e33ca

replace squish with strip again due to failed rebase :(

bbd485b

extend describe_vpcs tests

ead2e8a

improve VPC mocked mode; fix tests

915e069

ddiachkov force-pushed the fix_vpc_parser branch from dd1de0b to 915e069 Compare October 17, 2017 22:12

lanej approved these changes Oct 17, 2017

View reviewed changes

geemus approved these changes Oct 18, 2017

View reviewed changes

geemus merged commit 298a99d into fog:master Oct 18, 2017

lanej mentioned this pull request Mar 14, 2018

Ability to save cidrBlockAssociationSet #386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VPC parser #389

Fix VPC parser #389

ddiachkov commented Sep 11, 2017

geemus left a comment

geemus Sep 12, 2017

ddiachkov Sep 14, 2017

geemus Sep 15, 2017

geemus Sep 12, 2017

ddiachkov Sep 14, 2017 •

edited

Loading

geemus Sep 15, 2017

geemus Sep 12, 2017

geemus commented Sep 12, 2017

lanej commented Sep 25, 2017

geemus commented Sep 25, 2017

lanej commented Sep 25, 2017

ddiachkov commented Oct 1, 2017

lanej commented Oct 5, 2017 •

edited

Loading

lanej commented Oct 5, 2017

ddiachkov commented Oct 5, 2017

lanej commented Oct 5, 2017 •

edited

Loading

ddiachkov commented Oct 5, 2017

ddiachkov commented Oct 6, 2017

ddiachkov commented Oct 6, 2017

lanej commented Oct 6, 2017

geemus commented Oct 17, 2017

lanej commented Oct 17, 2017

ddiachkov commented Oct 17, 2017

lanej commented Oct 17, 2017

geemus commented Oct 18, 2017

geemus commented Oct 18, 2017


		attribute :cidr_block_association_set, :aliases => 'cidrBlockAssociationSet'

		attribute :ipv_6_cidr_block_association_set, :aliases => 'ipv6CidrBlockAssociationSet'

Fix VPC parser #389

Fix VPC parser #389

Conversation

ddiachkov commented Sep 11, 2017

geemus left a comment

Choose a reason for hiding this comment

geemus Sep 12, 2017

Choose a reason for hiding this comment

ddiachkov Sep 14, 2017

Choose a reason for hiding this comment

geemus Sep 15, 2017

Choose a reason for hiding this comment

geemus Sep 12, 2017

Choose a reason for hiding this comment

ddiachkov Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

geemus Sep 15, 2017

Choose a reason for hiding this comment

geemus Sep 12, 2017

Choose a reason for hiding this comment

geemus commented Sep 12, 2017

lanej commented Sep 25, 2017

geemus commented Sep 25, 2017

lanej commented Sep 25, 2017

ddiachkov commented Oct 1, 2017

lanej commented Oct 5, 2017 • edited Loading

lanej commented Oct 5, 2017

ddiachkov commented Oct 5, 2017

lanej commented Oct 5, 2017 • edited Loading

ddiachkov commented Oct 5, 2017

ddiachkov commented Oct 6, 2017

ddiachkov commented Oct 6, 2017

lanej commented Oct 6, 2017

geemus commented Oct 17, 2017

lanej commented Oct 17, 2017

ddiachkov commented Oct 17, 2017

lanej commented Oct 17, 2017

geemus commented Oct 18, 2017

geemus commented Oct 18, 2017

ddiachkov Sep 14, 2017 •

edited

Loading

lanej commented Oct 5, 2017 •

edited

Loading

lanej commented Oct 5, 2017 •

edited

Loading