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

Fixes #5802 - HC update/create using system uuids #4175

Merged

Conversation

dustints
Copy link
Member

Alter Host Collection api update/create actions to accept system uuids.
This is a fix related to hammer-cli-katello commands where some of
the commands take uuids and others take the data base id. This allows
the cli commands to uniformly take uuids.

related to Katello/hammer-cli-katello#178

@dustints
Copy link
Member Author

thanks! @parthaa. I never would have figured out how to solve the issue causing the failing test.

systems = System.where(:uuid => uuids)
ids_not_found = Set.new(uuids).subtract(systems.map(&:uuid))
fail Errors::NotFound.new(_("Systems [%s] not found.") % ids_not_found.to_a.join(',')) unless ids_not_found.blank?
systems.collect { |s| s.id }
Copy link
Member

Choose a reason for hiding this comment

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

The mechanics of converting a set of uuids to ids seems like a good model layer method. Also, you should be able to just do a systems.pluck(:id) instead of a collect since this is an active record query.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ehelms thanks, moved system_uuids_to_ids to System.uuids_to_ids with test, and updated to use pluck.

@ehelms
Copy link
Member

ehelms commented May 30, 2014

@dustint-rh is the UI sending UUIDs?

@dustints
Copy link
Member Author

@ehelms currently in the ui:
on content-host creation, ui doesn't send systems to be created
when adding a system to the content-host, ui uses the add_systems action which only takes uuids
when removing a system, ui uses the remove_systems action which also only takes uuids.
update action isn't used by ui to add/remove systems.

This is mostly a change to help cli commands be more consistent by only accepting and displaying uuids.
I think what was suggested by @thomasmckay as a long term solution was to do away with all uuids and only deal in database ids. However, this would be a huge change and he had some concerns with making the things unstable.

@@ -62,6 +62,7 @@ class System < Katello::Model

scope :in_environment, lambda { |env| where('environment_id = ?', env) unless env.nil?}
scope :completer_scope, lambda { |options| readable(options[:organization_id])}
scope :by_uuids, lambda {|uuids| where(:uuid => uuids) }
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - equivalent spacing for brackets (currently trailing space but no leading).

@ehelms
Copy link
Member

ehelms commented Jun 2, 2014

ACK pending minor comment

Alter Host Collection api update/create actions to accept system uuids.
This is a fix related to hammer-cli-katello commands where some of
the commands take uuids and others take the data base id. This allows
the cli commands to uniformly take uuids.

* also added class_name to relation for system in SystemHostCollection
@dustints
Copy link
Member Author

dustints commented Jun 2, 2014

ty, updated the spacing

thomasmckay added a commit that referenced this pull request Jun 2, 2014
…_uuid

Fixes #5802 - HC update/create using system uuids
@thomasmckay thomasmckay merged commit 3eee12c into Katello:master Jun 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants