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

API calls to fetch and update the Automate Workspace #21

Merged
merged 9 commits into from
Sep 25, 2017

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Aug 15, 2017

Added API calls to fetch and update the Automate Workspace.
Each Automate Workspace has a unique GUID and they can be only retrieved using the GUID.

@mkanoor
Copy link
Contributor Author

mkanoor commented Aug 17, 2017

@mkanoor
Copy link
Contributor Author

mkanoor commented Aug 17, 2017

Travis failure is because of the other PR's not being merged

current_output = obj.output || {}
obj.output = current_output.deep_merge(data)
obj.save
obj
Copy link
Member

Choose a reason for hiding this comment

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

None of this belongs in a controller. The api controllers should be stupid, and the model should have all of this "how to patch an atuomate workspace" knowledge.

Copy link
Member

Choose a reason for hiding this comment

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

👍 model/business logic stays in model.

Copy link
Contributor

Choose a reason for hiding this comment

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

The api controllers should be stupid

❤️ ❤️ ❤️

@mkanoor mkanoor changed the title [WIP] API calls to fetch and update the Automate Workspace API calls to fetch and update the Automate Workspace Aug 30, 2017
@miq-bot miq-bot removed the wip label Aug 30, 2017
@@ -18,7 +18,7 @@ def edit_resource(_type, id, data = {})
if obj.nil?
raise NotFoundError, "Invalid Workspace #{id} specified"
end
obj.update_output(data)
obj.output = data
Copy link
Member

Choose a reason for hiding this comment

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

do we need a begin/rescue here ?

@imtayadeway
Copy link
Contributor

Hi! It looks like your PR has been affected by the recently merged #40, and as a result will need to be rebased. First, I apologize for any inconvenience caused. After rebasing, you'll need to update some specs in order for them to pass. In particular you'll need to change any path helpers, applying the following pattern:

vms_url => api_vms_url
vms_url(vm.id) => api_vm_url(nil, vm) 
vms_url(vm.compressed_id) => api_vm_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags" => api_vm_tags_url(nil, vm)
"#{vms_url(vm.compressed_id}/tags" => api_vm_tags_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags/#{tag.id}" => api_vm_tag_url(nil, vm, tag)
"#{vms_url(vm.compressed_id)}/tags/#{tag.compressed_id}" => api_vm_tag_url(nil, vm.compressed_id, tag.compressed_id)

If you run into any issues, please ping me and I will try to help you out.

Many thanks!
Tim

@miq-bot
Copy link
Member

miq-bot commented Sep 18, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Sep 22, 2017

Checked commits mkanoor/manageiq-api@b999edd~...b04eccf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti abellotti added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 25, 2017
@abellotti
Copy link
Member

LGTM!! We'll update this once we have the update in the base to support arbitrary resource identifier (i.e. guid instead of id).

@abellotti abellotti merged commit 55e8ead into ManageIQ:master Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants