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

[WIP] Azure dependencies updates, some fog-core-2.0 compatibility #407

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

stiller-leser
Copy link

Hi,

I have done some work on the Gem (limited to my purposes). This PR is purely informal and shouldn't be merged. It contains the work of #362, some work for fog-core-2.1 compatibility, an inheritance change for Server (So that the public ip address could be retrieved and ssh as well as sshable could be used), plus an update to the azure dependencies (and the related changes to the Compute, Network as well as Storage modules). That being said however, all other modules will currently not work (but should easily be adapated).

I tried to keep downward compatibility, e.g. for changes to managed disks, however I can not guarantee that I missed something.

Due to time restrictions I won't be able to continue work on other modules, but please feel free to ping me for feedback.

Regards,
stiller-leser

address_space.address_prefixes = DEFAULT_ADDRESS_PREFIXES
else
address_space = Azure::ARM::Network::Models::AddressSpace.new
address_space = Azure::Network::Profiles::Latest::Mgmt::Models::AddressSpace.new

Choose a reason for hiding this comment

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

Style/IdenticalConditionalBranches: Move address_space = Azure::Network::Profiles::Latest::Mgmt::Models::AddressSpace.new out of the conditional.

virtual_network.location = location
virtual_network.tags = tags

if address_prefixes.nil? || !address_prefixes.any?
address_space = Azure::ARM::Network::Models::AddressSpace.new
address_space = Azure::Network::Profiles::Latest::Mgmt::Models::AddressSpace.new

Choose a reason for hiding this comment

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

Style/IdenticalConditionalBranches: Move address_space = Azure::Network::Profiles::Latest::Mgmt::Models::AddressSpace.new out of the conditional.

resource_group,
nic.public_ip_address_id.split("/").last
)
.ip_address

Choose a reason for hiding this comment

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

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.

.public_ips
.get(
resource_group,
nic.public_ip_address_id.split("/").last

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

@public_ip_addresses.push(
network_service
.public_ips
.get(

Choose a reason for hiding this comment

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

Layout/DotPosition: Place the . on the previous line, together with the method call receiver.

def network_service
storage_service = service.instance_variable_get(:@storage_service)
@network_Service ||= Fog::Network::AzureRM.new({
tenant_id: storage_service.instance_variable_get(:@tenant_id),

Choose a reason for hiding this comment

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

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

@@ -195,8 +198,54 @@ def update_attributes
merge_attributes(Fog::Compute::AzureRM::Server.parse(vm))
end

def network_service
storage_service = service.instance_variable_get(:@storage_service)
@network_Service ||= Fog::Network::AzureRM.new({

Choose a reason for hiding this comment

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

Naming/MemoizedInstanceVariableName: Memoized variable @network_Service does not match method name network_service. Use @network_service instead.
Naming/VariableName: Use snake_case for variable names.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

@@ -1,9 +1,11 @@
require "fog/compute/models/server"

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

end

def ready?
provisioning_state == "Succeeded"

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

self.sku = {
name: self.account_type
}
elsif self.account_type.nil? && self.sku.nil?

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

rescue MsRestAzure::AzureOperationError => e
raise_azure_exception(e, msg)
end
Fog::Logger.debug "listing all Virtual Machines in subscription successful"

Choose a reason for hiding this comment

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

Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

# Monkey patch for fog-core-2 compatibility
if disk_name.nil?
disks = service.list_managed_disks_in_subscription
disk = disks.each do |disk|

Choose a reason for hiding this comment

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

Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - disk.


if self.sku.nil? && !self.account_type.nil?
self.sku = {
name: self.account_type

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

validate_creation_data_params(creation_data)
requires :disk_size_gb

if self.sku.nil? && !self.account_type.nil?

Choose a reason for hiding this comment

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

Style/RedundantSelf: Redundant self detected.

@apuntamb
Copy link

Hi @stiller-leser . I am currently working on integrating Foreman and AzureRM. In the process, I am also looking into fog 2.1.0 compatibilty and while testing for the same, I discovered that the dependencies in fog-azure-rm.gemspec need an upgrade for the bundle to be successfully updated which you have covered in this PR. Any chance this can be merged sooner or if it's in WIP, should I open a PR specific to gemspec dependency upgrade?

@stiller-leser
Copy link
Author

@apuntamb Just as a headsup, as this PR likely will never be merged, make sure to take a look at my changes for Foreman. The new Azure sdk versions break some things for disk creation.

private

def validate_creation_data_params(creation_data)
unless creation_data.key?(:create_option)
raise(ArgumentError, ':create_option is required for this operation')
if !creation_data || creation_data.create_option.nil?

Choose a reason for hiding this comment

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

Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.

private

def validate_creation_data_params(creation_data)
if !creation_data || creation_data.create_option.nil?

Choose a reason for hiding this comment

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

Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.

end

def test_model_attributes
attributes = %i(

Choose a reason for hiding this comment

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

Style/PercentLiteralDelimiters: %i-literals should be delimited by [ and ].

end

def test_collection_methods
methods = %i(all get)

Choose a reason for hiding this comment

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

Style/PercentLiteralDelimiters: %i-literals should be delimited by [ and ].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants