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

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

Merged
merged 4 commits into from
Apr 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions fog-aws.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ Gem::Specification.new do |spec|

spec.add_development_dependency 'bundler', '~> 1.15'
spec.add_development_dependency 'rake', '~> 10.0'
spec.add_development_dependency 'shindo', '~> 0.3'
spec.add_development_dependency 'rubyzip', '~> 1.2.1'
spec.add_development_dependency 'shindo', '~> 0.3'

spec.add_dependency 'fog-core', '~> 1.38'
spec.add_dependency 'fog-json', '~> 1.0'
spec.add_dependency 'fog-core', '~> 2.1'
spec.add_dependency 'fog-json', '~> 1.1'
spec.add_dependency 'fog-xml', '~> 0.1'
spec.add_dependency 'ipaddress', '~> 0.8'
end
41 changes: 14 additions & 27 deletions lib/fog/aws/models/compute/volume.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,19 @@ def save
requires :availability_zone
requires_one :size, :snapshot_id

if type == 'io1'
requires :iops
end
requires :iops if type == 'io1'

data = service.create_volume(availability_zone, size, create_params).body
merge_attributes(data)

if tags = self.tags
# expect eventual consistency
Fog.wait_for { self.reload rescue nil }
service.create_tags(
self.identity,
tags
)
Fog.wait_for { service.volumes.get(identity) }
service.create_tags(identity, tags)
end

if @server
self.server = @server
end
attach(@service, device) if @server

true
end
end
Expand All @@ -87,13 +81,7 @@ def server
service.servers.get(server_id)
end

def server=(new_server)
if new_server
attach(new_server)
else
detach
end
end
attr_writer :server

def snapshots
requires :id
Expand All @@ -109,22 +97,15 @@ def force_detach
detach(true)
end

private

def attachmentSet=(new_attachment_set)
merge_attributes(new_attachment_set.first || {})
end

def attach(new_server)
def attach(new_server, new_device)
Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

if !persisted?
@server = new_server
self.availability_zone = new_server.availability_zone
elsif new_server
requires :device
wait_for { ready? }
@server = nil
self.server_id = new_server.id
service.attach_volume(server_id, id, device)
service.attach_volume(server_id, id, new_device)
reload
end
end
Expand All @@ -138,6 +119,12 @@ def detach(force = false)
end
end

private

def attachmentSet=(new_attachment_set)
merge_attributes(new_attachment_set.first || {})
end

def create_params
{
'Encrypted' => encrypted,
Expand Down
2 changes: 1 addition & 1 deletion lib/fog/aws/models/elb/backend_server_descriptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def all
end

def get(instance_port)
all.find{|e| e.instance_port == instance_port}
all.find { |e| e.instance_port == instance_port }
end
end
end
Expand Down
7 changes: 3 additions & 4 deletions lib/fog/aws/models/elb/listeners.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,16 @@ def all
end

def get(lb_port)
all.find{|listener| listener.lb_port == lb_port}
all.find { |listener| listener.lb_port == lb_port }
end

private

# Munge an array of ListenerDescription hashes like:
# {'Listener' => listener, 'PolicyNames' => []}
# to an array of listeners with a PolicyNames key
def munged_data
data.map {|description|
description['Listener'].merge('PolicyNames' => description['PolicyNames'])
}
data.map { |description| description['Listener'].merge('PolicyNames' => description['PolicyNames']) }
end
end
end
Expand Down
12 changes: 10 additions & 2 deletions lib/fog/aws/models/elb/load_balancer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -223,16 +223,24 @@ def save
end

def reload
super
@instance_health = nil
@policy_descriptions = nil
self
super
end

def destroy
requires :id
service.delete_load_balancer(id)
end

protected

def all_associations_and_attributes
super.merge(
'ListenerDescriptions' => attributes['ListenerDescriptions'],
'BackendServerDescriptions' => attributes['BackendServerDescriptions'],
)
end
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@lanej lanej Mar 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

end
end
end
Expand Down
11 changes: 5 additions & 6 deletions lib/fog/aws/models/elb/load_balancers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ class LoadBalancers < Fog::Collection
model Fog::AWS::ELB::LoadBalancer

# Creates a new load balancer
def initialize(attributes={})
def initialize(attributes = {})
super
end

def all
result = []
marker = nil
finished = false
while !finished
until finished
data = service.describe_load_balancers('Marker' => marker).body
result.concat(data['DescribeLoadBalancersResult']['LoadBalancerDescriptions'])
marker = data['DescribeLoadBalancersResult']['NextMarker']
Expand All @@ -24,10 +24,9 @@ def all
end

def get(identity)
if identity
data = service.describe_load_balancers('LoadBalancerNames' => identity).body['DescribeLoadBalancersResult']['LoadBalancerDescriptions'].first
new(data)
end
return unless identity
data = service.describe_load_balancers('LoadBalancerNames' => identity).body['DescribeLoadBalancersResult']['LoadBalancerDescriptions'].first
new(data)
rescue Fog::AWS::ELB::NotFound
nil
end
Expand Down
45 changes: 24 additions & 21 deletions lib/fog/aws/requests/elb/create_load_balancer_listeners.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,34 +51,37 @@ def create_load_balancer_listeners(lb_name, listeners)

class Mock
def create_load_balancer_listeners(lb_name, listeners)
if load_balancer = self.data[:load_balancers][lb_name]
response = Excon::Response.new
load_balancer = data[:load_balancers][lb_name]
raise Fog::AWS::ELB::NotFound unless load_balancer
response = Excon::Response.new

certificate_ids = Fog::AWS::IAM::Mock.data[@aws_access_key_id][:server_certificates].map {|n, c| c['Arn'] }
certificate_ids = Fog::AWS::IAM::Mock.data[@aws_access_key_id][:server_certificates].map { |_n, c| c['Arn'] }

listeners.each do |listener|
if listener['SSLCertificateId'] and !certificate_ids.include? listener['SSLCertificateId']
raise Fog::AWS::IAM::NotFound.new('CertificateNotFound')
end
listeners.each do |listener|
if listener['SSLCertificateId'] && !certificate_ids.include?(listener['SSLCertificateId'])
raise Fog::AWS::IAM::NotFound, 'CertificateNotFound'
end

if (%w( HTTP HTTPS).include?(listener['Protocol']) && !%w( HTTP HTTPS ).include?(listener['InstanceProtocol'])) ||
(%w( TCP SSL).include?(listener['Protocol']) && !%w( TCP SSL ).include?(listener['InstanceProtocol']))
raise Fog::AWS::ELB::ValidationError
end if listener['Protocol'] && listener['InstanceProtocol']
load_balancer['ListenerDescriptions'] << {'Listener' => listener, 'PolicyNames' => []}
if listener['Protocol'] && listener['InstanceProtocol']
if (
%w[HTTP HTTPS].include?(listener['Protocol']) && !%w[HTTP HTTPS].include?(listener['InstanceProtocol'])
) || (
%w[TCP SSL].include?(listener['Protocol']) && !%w[TCP SSL].include?(listener['InstanceProtocol'])
)
raise Fog::AWS::ELB::ValidationError
end
end
load_balancer['ListenerDescriptions'] << { 'Listener' => listener, 'PolicyNames' => [] }
end

response.status = 200
response.body = {
'ResponseMetadata' => {
'RequestId' => Fog::AWS::Mock.request_id
}
response.status = 200
response.body = {
'ResponseMetadata' => {
'RequestId' => Fog::AWS::Mock.request_id
}
}

response
else
raise Fog::AWS::ELB::NotFound
end
response
end
end
end
Expand Down
25 changes: 17 additions & 8 deletions tests/models/compute/volume_tests.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
Shindo.tests("Fog::Compute[:aws] | volume", ['aws']) do

Shindo.tests('Fog::Compute[:aws] | volume', ['aws']) do
@server = Fog::Compute[:aws].servers.create
@server.wait_for { ready? }

model_tests(Fog::Compute[:aws].volumes, {:availability_zone => @server.availability_zone, :size => 1, :device => '/dev/sdz1', :tags => {"key" => "value"}, :type => 'gp2'}, true) do
model_tests(
Fog::Compute[:aws].volumes,
{
availability_zone: @server.availability_zone,
size: 1,
tags: { 'key' => 'value' },
type: 'gp2'
},
true
) do

@instance.wait_for { ready? }

tests('#server = @server').succeeds do
@instance.server = @server
tests('#attach(server, device)').succeeds do
@instance.attach(@server, '/dev/sdz1')
end

@instance.wait_for { state == 'in-use' }
Expand All @@ -17,13 +25,14 @@
@instance.server.id == @server.id
end

tests('#server = nil').succeeds do
(@instance.server = nil).nil?
tests('#detach').succeeds do
@instance.detach
@instance.server.nil?
end

@instance.wait_for { ready? }

@instance.server = @server
@instance.attach(@server, '/dev/sdz1')
@instance.wait_for { state == 'in-use' }

tests('#force_detach').succeeds do
Expand Down