fog-core 2.x, fog-json 1.x #433

lanej · 2018-02-26T17:59:03Z

References #227

geemus · 2018-02-27T14:36:41Z

Looks good in theory, seems there are a couple breakages though, unfortunately.

References #227

ListenerDescriptions and BackendServerDescriptions are used to build ad-hoc collections but are not registered as attributes of the LoadBalancer model.

plribeiro3000 · 2018-03-14T23:20:51Z

lib/fog/aws/models/elb/load_balancer.rb

+            'ListenerDescriptions' => attributes['ListenerDescriptions'],
+            'BackendServerDescriptions' => attributes['BackendServerDescriptions'],
+          )
+        end


Why do you need this? Maybe we need to improve this method on fog-core?

Attributes are not registered by an attribute declaration but are used to generate to collections from the underlying attributes data.

This seems like a reasonable workaround using normal OO methodology.

Got it. I'm not really pointing this as a problem but i'm more interested in knowing the reasons behind it. If it does inflect an in improvement in fog-core i would be more then happy to do it myself. =)

Are those attributes not being used by the DSL for a particular reason? Is there something i can improve in fog-core to help improve the usage?

They seem to describe related resources but not this resource, which is my they are not included as attributes. If other providers are having this issue, we could introduce a "hidden attribute" (data we want to keep but does not describe the current model).

Hmmm. So you mean an attribute that ideally does not belong to this resource but the API expects? Like a concept issue from the API standpoint?

BTW, this is not a blocker. I'm just trying to figure out the use case to see if/how we can improve fog-core. My main goal now is to get it to a better state.

jaredbeck · 2018-04-03T20:11:03Z

Copying the test failure here for convenience:

      tests/models/compute/volume_tests.rb
      MissingParameter => The request must contain the parameter device (Fog::Compute::AWS::Error)
        /home/travis/build/fog/fog-aws/lib/fog/aws/requests/compute/attach_volume.rb:77:in `attach_volume'
        /home/travis/build/fog/fog-aws/lib/fog/aws/models/compute/volume.rb:127:in `attach'
        /home/travis/build/fog/fog-aws/lib/fog/aws/models/compute/volume.rb:92:in `server='
        tests/models/compute/volume_tests.rb:11:in `block (3 levels) in <top (required)>'
...

Question: Is this test failure the result of breaking changes in the updated dependencies (eg. fog-core) or is it a result of the preceding warning:

  AWS | credentials (aws) +++++[fog][WARNING] Unable to fetch credentials: Expected(200) <=> Actual(404 Not Found)

I'm not seeing any mention of "volumes" in the fog-core changelog There were actually very few changes between 1.45 and 2.0

564ea20 geemus     2018-01-03 09:23:05 -0600  (tag: v2.0.0) v2.0.0
f5521c2 Paul..eiro 2017-11-13 15:55:42 -0200  Merge pull request #221 from NARKOZ/mime-types
94f76f7 Niha..asov 2017-11-13 16:27:30 +0400  Don't check mime/types for load error
a32e960 Niha..asov 2017-11-13 12:49:55 +0400  Add missing runtime dependency declaration to gemspec
65e33d0 Paul..eiro 2017-10-27 11:14:16 -0200  Merge pull request #220 from JustMcAfee/master
e7e0657 JustMcAfee 2017-10-27 08:30:41 -0400  Updating net-ssh parameter names
9fabe1c Wesl..eary 2017-10-26 08:36:45 -0500  Merge pull request #219 from tbrisker/ruby2
5c53ee1 Tome..sker 2017-10-24 11:48:27 +0300  Drop Ruby<2 support
1af65f0 Paul..eiro 2017-10-02 17:54:56 -0300  Merge pull request #218 from fog/fix-reload-for-associations
85cfeae Paul..eiro 2017-09-30 15:12:53 -0300  fix association reload
13979be geemus     2017-08-01 11:46:25 -0500  (tag: v1.45.0) v1.45.0

geemus · 2018-04-05T20:47:05Z

I don't think fog-core has a notion of volumes at all, so I don't think it would be that. Other changes to fog-core could be causing this, I suppose, but I'm not sure which one specifically. I suppose the association reload fix might relate, but I'm not at all certain about that (none of the others look particularly related).

jaredbeck · 2018-04-05T21:20:09Z

What should we make of the "Unable to fetch credentials: Expected(200) <=> Actual(404 Not Found)" error? Do you think it's related to the test failure in volume_tests.rb? Sorry if these are noob questions, I don't know much about fog yet.

geemus · 2018-04-07T15:36:43Z

@jaredbeck no worries. I think it is probably unrelated (and it's just a warning). Most likely there is an actual issue in the parameters passed within that attach volumes call.

new fog-core model#reload resets the model the current remote state. The previous implementation of volume relied upon initializing with a device, reloading the model several times, and then attaching with a server instance is set. IMO, the new fog-core is doing the correct thing. To support this behavior, make #attach and #detach public methods and require a device argument in #attach. #server= does exist but only as an attr_writer. This may cause some confusion in the future release and must be included in the CHANGELOG.

lanej · 2018-04-09T20:37:50Z

@geemus @plribeiro3000

got it working. needed to change the volume interface slightly, but I think it's for the better.

plribeiro3000 · 2018-04-09T22:28:02Z

LGTM!

geemus · 2018-04-10T14:14:23Z

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

-        end
-
-        def attach(new_server)
+        def attach(new_server, new_device)


Could you elaborate on why the new argument is needed? The rest looks good, but I don't think I'm quite groking this. Thanks!

Tests asserted this procedure

v = service.volumes.create(device: '/dev/sda1', **)

v.server = service.instances.create(**) # requires :device

In step 1, a device is provided to a volume instance. In step 2, the model calls Fog.wait_for { ready? } (implicit reload) and then attaches the server.

The issue is between step 1 and 2, there is a property of the local model (device = '/dev/sda1') that does not match the remote model (device = nil).

On reload:

fog-core 1.x, the device attribute retained it's value.

fog-core 2.x, device is set the correct remote value (nil).

IMO a reload should put the local model in the same state as the remote model. To enforce that, I've removed the ability for volume.server = server to attach volume and instead require calling volume.attach(server, device).

geemus

Sounds like a plan, thanks for clarifying.

jaredbeck · 2018-04-16T17:46:53Z

Thanks @lanej et al. for your work on this.

lanej mentioned this pull request Mar 7, 2018

fix nil fetch on object reload fog/fog-core#228

Merged

fog-core 2.x, fog-json 1.x

ec620d8

References #227

lanej force-pushed the lanej/fog-dep-pins branch from 56480c0 to ec620d8 Compare March 14, 2018 23:15

lanej added 2 commits March 14, 2018 16:16

fix(elb): override #all_associations_and_attributes

199cbf8

ListenerDescriptions and BackendServerDescriptions are used to build ad-hoc collections but are not registered as attributes of the LoadBalancer model.

style(elb): reduce branch conditions and increase readability

770629e

lanej mentioned this pull request Mar 14, 2018

Allow fog-core 2.0 #429

Closed

plribeiro3000 reviewed Mar 14, 2018

View reviewed changes

geemus reviewed Apr 10, 2018

View reviewed changes

geemus approved these changes Apr 16, 2018

View reviewed changes

geemus merged commit e8856b7 into master Apr 16, 2018

geemus deleted the lanej/fog-dep-pins branch April 16, 2018 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fog-core 2.x, fog-json 1.x #433

fog-core 2.x, fog-json 1.x #433

lanej commented Feb 26, 2018

geemus commented Feb 27, 2018

plribeiro3000 Mar 14, 2018

lanej Mar 14, 2018 •

edited

Loading

plribeiro3000 Mar 15, 2018

lanej Apr 9, 2018

plribeiro3000 Apr 9, 2018

jaredbeck commented Apr 3, 2018

geemus commented Apr 5, 2018

jaredbeck commented Apr 5, 2018

geemus commented Apr 7, 2018

lanej commented Apr 9, 2018

plribeiro3000 commented Apr 9, 2018

geemus Apr 10, 2018

lanej Apr 10, 2018

geemus left a comment

jaredbeck commented Apr 16, 2018

fog-core 2.x, fog-json 1.x #433

fog-core 2.x, fog-json 1.x #433

Conversation

lanej commented Feb 26, 2018

geemus commented Feb 27, 2018

plribeiro3000 Mar 14, 2018

Choose a reason for hiding this comment

lanej Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

plribeiro3000 Mar 15, 2018

Choose a reason for hiding this comment

lanej Apr 9, 2018

Choose a reason for hiding this comment

plribeiro3000 Apr 9, 2018

Choose a reason for hiding this comment

jaredbeck commented Apr 3, 2018

geemus commented Apr 5, 2018

jaredbeck commented Apr 5, 2018

geemus commented Apr 7, 2018

lanej commented Apr 9, 2018

plribeiro3000 commented Apr 9, 2018

geemus Apr 10, 2018

Choose a reason for hiding this comment

lanej Apr 10, 2018

Choose a reason for hiding this comment

geemus left a comment

Choose a reason for hiding this comment

jaredbeck commented Apr 16, 2018

lanej Mar 14, 2018 •

edited

Loading