tonobo / hcloud-ruby

Native ruby client for HetznerCloud
MIT License
32 stars 11 forks source link

(#82) fix undefined method `.blank?` #84

Closed bastelfreak closed 8 months ago

bastelfreak commented 1 year ago

This adds the activesupport dependency which provides .blank?. It's used in a lot of places so I added the dependency instead of switching to native ruby.

$ git grep .blank?
lib/hcloud/certificate_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no certificate given' if certificate.blank?
lib/hcloud/certificate_resource.rb:        raise Hcloud::Error::InvalidInput, 'no private_key given' if private_key.blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no type given' if kwargs[:type].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no start given' if kwargs[:start].blank?
lib/hcloud/entry_loader.rb:          raise Hcloud::Error::InvalidInput, 'no end given' if kwargs[:end].blank?
lib/hcloud/firewall_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/floating_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/floating_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/load_balancer.rb:      raise Hcloud::Error::InvalidInput, 'no protocol given' if protocol.blank?
lib/hcloud/load_balancer.rb:        raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/load_balancer_resource.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if load_balancer_type.blank?
lib/hcloud/load_balancer_resource.rb:      if !algorithm.to_h.key?(:type) || algorithm[:type].blank?
lib/hcloud/load_balancer_resource.rb:      if location.blank? && network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no type given' if type.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no network_zone given' if network_zone.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no destination given' if destination.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no gateway given' if gateway.blank?
lib/hcloud/network.rb:      raise Hcloud::Error::InvalidInput, 'no ip_range given' if ip_range.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/network_resource.rb:      raise Hcloud::Error::InvalidInput, 'no IP range given' if ip_range.blank?
lib/hcloud/placement_group_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/primary_ip_resource.rb:      raise Hcloud::Error::InvalidInput, 'no assignee_type given' if assignee_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no image given' if image.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no server_type given' if server_type.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no iso given' if iso.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no IP given' if ip.blank?
lib/hcloud/server.rb:      raise Hcloud::Error::InvalidInput, 'no dns_ptr given' if dns_ptr.blank?
lib/hcloud/ssh_key_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      raise Hcloud::Error::InvalidInput, 'no name given' if name.blank?
lib/hcloud/volume_resource.rb:      if location.blank? && server.nil?
ghost commented 8 months ago

I have been wondering why you set a version restriction spec.add_runtime_dependency 'activesupport', '~> 7.1', '>= 7.1.1' in the gemspec. Are previous versions of activesupport broken or somehow different?

The new line require 'active_support/core_ext/object/blank' looks fine, only the version restriction is confusing me (if it has no real reason).

bastelfreak commented 8 months ago

As far as I learned it's good practice to use explicit pinning to ensure we don't pull in old versions that don't work and also don't pull in new major versions. 7.1.1 was the latest version when I wrote the patch.

ghost commented 8 months ago

Hm... But doesn't the gemspec transitively expand also on downstream users of this library? I don't think we should force other projects to use only activesupport '>= 7.1.1' (unless we know for sure that our library does not work with versions below 7.1.1).

I usually like to keep libraries as broad as possible and pin applications strictly (what Gemfile.lock does in the Ruby world). In libraries I only add version restrictions if they're absolutely necessary. If libraries used version restrictions often, it could easily happen that an applications wants to use two libraries but they have incompatible version restrictions.

I have one project that uses this library and added a restriction activesupport < 7.1 there to simulate that maybe (for whatever reason) that project only works with older versions of activesupport.

With the current release of hcloud-ruby that works without problems. It pins activesupport to activesupport (7.0.8) in Gemfile.lock. If I use this PR of hcloud-ruby it leads to a version resolution error:

Could not find compatible versions

Because every version of [myapplication] depends on activesupport < 7.1
  and every version of hcloud depends on activesupport >= 7.1.1, < 8.A

If you can remove the version restriction, I could merge it.

bastelfreak commented 8 months ago

@aufziehvogel are you fine with the upper limit to prevent pulling int major releases that might break hcloud? with the dependabot plugin you will get an automatic PR that will run tests and bump the version, so you won't miss it.

ghost commented 8 months ago

@tonobo What do you think? I'd also allow future major versions (in this case activesupport 8 or later), because the chances that our code still runs with activesupport 8 without problems are pretty high. Or in other words, I'd only put a restriction if I really know that we're broken with a specific version.

tonobo commented 8 months ago

Howdy, applying a static pinning via gemspec is not what we commonly do, it's up to your project to pin dependencies (usually via Gemfile.lock). I personally dislike introducing hard dependencies at gem level, guess the project isn't maintained anymore but still enforcing to stick to an old ruby versions. grpc gem did something like this in the past which blocked our internal upgrade procedures around christmas because everybody there was on vacation. There are definitely cons to do so, but I'd like to stick to the actual no pinning way. Maybe @Kjarrigan or @aufziehvogel might have a different opinion there.

ghost commented 8 months ago

Same opinion from my side. I also prefer no pinnings in gemspec.

bastelfreak commented 8 months ago

@skoch-hc thanks for merging, can you also do a new release?

skoch-hc commented 8 months ago

Have released version 1.3.0 to rubygems.