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

Models should not have more than 1 required parameter in get() #352

Open
3 of 6 tasks
Temikus opened this issue Jun 5, 2018 · 5 comments
Open
3 of 6 tasks

Models should not have more than 1 required parameter in get() #352

Temikus opened this issue Jun 5, 2018 · 5 comments
Assignees
Labels
Milestone

Comments

@Temikus
Copy link
Member

Temikus commented Jun 5, 2018

This will need to be fixed, otherwise lifecycle doesn't match:

λ grep -R "def get([a-z_\*-]*, " . 
./lib/fog/google/models/sql/ssl_certs.rb:        def get(instance_id, sha1_fingerprint)
./lib/fog/google/models/sql/backup_runs.rb:        def get(instance_id, backup_run_id)
./lib/fog/storage/google_xml/models/directories.rb:        def get(key, options = {})
./lib/fog/storage/google_xml/models/files.rb:        def get(key, options = {}, &block)
./lib/fog/storage/google_json/models/directories.rb:        def get(bucket_name, options = {})
./lib/fog/storage/google_json/models/files.rb:        def get(key, options = {}, &block)
./lib/fog/compute/google/models/target_pools.rb:        def get(identity, region = nil)
./lib/fog/compute/google/models/addresses.rb:        def get(identity, region)
./lib/fog/compute/google/models/instance_groups.rb:        def get(identity, zone = nil)
./lib/fog/compute/google/models/images.rb:        def get(identity, project = nil)
./lib/fog/compute/google/models/instance_group_managers.rb:        def get(identity, zone = nil)
./lib/fog/compute/google/models/forwarding_rules.rb:        def get(identity, region = nil)
./lib/fog/compute/google/models/operations.rb:        def get(identity, zone = nil, region = nil)
./lib/fog/compute/google/models/disks.rb:        def get(identity, zone)
./lib/fog/compute/google/models/servers.rb:        def get(identity, zone = nil)
./lib/fog/compute/google/models/machine_types.rb:        def get(identity, zone)
./lib/fog/compute/google/models/disk_types.rb:        def get(identity, zone = nil)
./lib/fog/compute/google/models/target_instances.rb:        def get(target_instance, zone = nil)
./lib/fog/compute/google/models/subnetworks.rb:        def get(identity, region)
./lib/fog/dns/google/models/records.rb:        def get(name, type)

Game plan:

  • Unit tests for basic model lifecycle methods.
  • Add lookup logic if the resource location is not provided (probably via all?)
  • Switch all zone/region parameters to be optional, but leave them in to speed up logic when needed.
  • Fixup the regression in collection tests (see Rethink the get() methods required parameters #396)
  • Unit tests checking for get() required parameters so this doesn't deviate again.
  • Remove skip test fixtures in test:unit
@Temikus
Copy link
Member Author

Temikus commented Jun 5, 2018

I think this may be a good time to think about some unit tests testing the most basic behavior of a model.

I.e. should respond to:
.save
.destroy

We can get all models loaded via:

ObjectSpace.each_object(Fog::Model.singleton_class) {|it| puts it}
Fog::Model
Fog::Compute::Google::GlobalAddress
Fog::Compute::Google::Zone
Fog::Compute::Google::Region
Fog::Compute::Google::HttpHealthCheck
Fog::Compute::Google::TargetPool
Fog::Compute::Google::ForwardingRule
Fog::Compute::Google::Project
Fog::Compute::Google::Firewall
Fog::Compute::Google::Network
Fog::Compute::Google::Route
Fog::Compute::Google::BackendService
Fog::Compute::Google::TargetHttpProxy
Fog::Compute::Google::TargetHttpsProxy
Fog::Compute::Google::UrlMap
Fog::Compute::Google::GlobalForwardingRule
Fog::Compute::Google::TargetInstance
Fog::Compute::Google::Image
Fog::Compute::Google::Disk
Fog::Compute::Google::MachineType
Fog::Compute::Google::Snapshot
Fog::Compute::Server
Fog::Compute::Google::Server
Fog::Compute::Google::DiskType
Fog::Compute::Google::Address
Fog::Compute::Google::Operation
Fog::Compute::Google::InstanceGroup
Fog::Compute::Google::Subnetwork
Fog::Compute::Google::InstanceTemplate
Fog::Compute::Google::InstanceGroupManager
Fog::Compute::Google::SslCertificate

Same with collection:

ObjectSpace.each_object(Fog::Collection.singleton_class) {|it| puts it}
Fog::Collection
Fog::Association
Fog::PagedCollection
Fog::Compute::Google::Servers
Fog::Compute::Google::Images
Fog::Compute::Google::Disks
Fog::Compute::Google::DiskTypes
Fog::Compute::Google::MachineTypes
Fog::Compute::Google::Addresses
Fog::Compute::Google::GlobalAddresses
Fog::Compute::Google::Operations
Fog::Compute::Google::Snapshots
Fog::Compute::Google::Regions
Fog::Compute::Google::Zones
Fog::Compute::Google::HttpHealthChecks
Fog::Compute::Google::TargetPools
Fog::Compute::Google::Projects
Fog::Compute::Google::ForwardingRules
Fog::Compute::Google::Firewalls
Fog::Compute::Google::Networks
Fog::Compute::Google::BackendServices
Fog::Compute::Google::Routes
Fog::Compute::Google::TargetHttpProxies
Fog::Compute::Google::TargetHttpsProxies
Fog::Compute::Google::GlobalForwardingRules
Fog::Compute::Google::UrlMaps
Fog::Compute::Google::TargetInstances
Fog::Compute::Google::InstanceGroups
Fog::Compute::Google::Subnetworks
Fog::Compute::Google::InstanceTemplates
Fog::Compute::Google::InstanceGroupManagers
Fog::Compute::Google::SslCertificates

@Temikus Temikus self-assigned this Jun 5, 2018
@Temikus
Copy link
Member Author

Temikus commented Jun 6, 2018

Note to self:

common_ancestors = [Fog::Collection, Fog::Association, Fog::PagedCollection]
descendants = ObjectSpace.each_object(Fog::Collection.singleton_class).to_a

collections = descendants - common_ancestors
common_ancestors = [Fog::Model]
descendants = ObjectSpace.each_object(Fog::Model.singleton_class).to_a

models = descendants - common_ancestors

@Temikus Temikus added this to the 2.0 milestone Jun 22, 2018
@Temikus Temikus changed the title Some compute models have more than 1 parameter in get() Models should not have more than 1 parameter in get() Jul 15, 2018
@icco
Copy link
Member

icco commented Jul 15, 2018

This sounds great!

Temikus added a commit to Temikus/fog-google that referenced this issue Jul 20, 2018
+ test fixtures for fog#352
Temikus added a commit to Temikus/fog-google that referenced this issue Jul 22, 2018
This should fix both fog#33 and fog#352 for compute
Temikus added a commit to Temikus/fog-google that referenced this issue Jul 22, 2018
This should fix both fog#33 and fog#352 for compute
Temikus added a commit to Temikus/fog-google that referenced this issue Jul 22, 2018
To test both scoped and unscoped code paths

See fog#352 for more context
@Temikus Temikus changed the title Models should not have more than 1 parameter in get() Models should not have more than 1 required parameter in get() Sep 21, 2018
@Temikus
Copy link
Member Author

Temikus commented Sep 26, 2018

Made some good progress on this in #400. SQL and DNS are the only collections left that have >1 required argument get methods.

@github-actions
Copy link

This issue has been marked inactive and will be closed if no further activity occurs.

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

No branches or pull requests

3 participants