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

Add delete automate domain support #548

Merged
merged 1 commit into from Feb 1, 2019

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Jan 25, 2019

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1645041

This PR adds support to allow removal of automate domains via the API
This change only supports deletion of unlocked user domains. System and
GIT based domains will not be deleted.

To test:

  • Create automated domains in the MiQ Database
  • Attempt to delete one of the automated domains in the MiQ Database using the API with the DELETE verb

e.g.:

curl --insecure --user <user>:<password> -X DELETE <appliance URL>/api/automate_domains/<domain name>

  • Attempt to delete one of the automated domains in the MiQ Database using the API with the POST verb

e.g.:

curl --insecure --user " <user>:<password>" -H "Content-Type: application/json" --data '{"action":"delete"}' -X POST <URL>/api/automate_domains/<domain name>

@jvlcek
Copy link
Member Author

jvlcek commented Jan 25, 2019

@miq-bot add_label enhancement

@jvlcek
Copy link
Member Author

jvlcek commented Jan 25, 2019

@miq-bot add_label hammer/no

@jvlcek
Copy link
Member Author

jvlcek commented Jan 25, 2019

@miq-bot assign @juliancheal

@jvlcek
Copy link
Member Author

jvlcek commented Jan 25, 2019

@mkanoor Please review

@jvlcek
Copy link
Member Author

jvlcek commented Jan 25, 2019

@gtanzillo Please review

@jvlcek jvlcek closed this Jan 28, 2019
@jvlcek
Copy link
Member Author

jvlcek commented Jan 28, 2019

rerunning build. Failures look unrelated.

@jvlcek jvlcek reopened this Jan 28, 2019
api_log_info("Deleting #{automate_domain_ident(domain)}")

begin
MiqAeDomain.where(:name => domain.name).each(&:destroy)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jvlcek I think we have something called readonly domains like ManageIQ and Redhat should the user be allowed to delete these domains. Or should this be allowed only for user domains?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jvlcek is this going to use some RBAC check to see if a user is allowed to delete a domain.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jvlcek I think instead of destroy it might be better to call destroy_queue since we would want to delete the git directories if the domain is a git based domain.
https://github.com/ManageIQ/manageiq-automation_engine/blob/e136b74e9b2005d050149d9c7019784a531d82e1/app/models/miq_ae_domain.rb#L162

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your help @mkanoor! It is very much appreciated.
I have pushed a change to address two of your three comments.
I will push the resolution the the RBAC check issue and an updated spec shortly.

JoeV

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkanoor RBAC is already being handled by the change to the config/api.yml file.
e.g.:
https://github.com/ManageIQ/manageiq-api/pull/548/files#diff-15502b585d5033aa8043cc26237a06baR257

:identifier: miq_ae_domain_delete

So the user must be associated with a role that has the miq_ae_domain_delete product feature enabled.

So RBAC is being check.

Thank you.


begin
# Do not delete the domain if any are locked
MiqAeDomain.where(:name => domain.name).each { |d| raise "Not deleting. Domain is locked." if d.contents_locked? }
Copy link
Contributor

Choose a reason for hiding this comment

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

@jvlcek Looks good, you might want to mention somewhere that we won't be deleting

  • System Domains
  • GIT Based domains

The only ones you can delete are user domains which are unlocked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. Thank you.

@jvlcek
Copy link
Member Author

jvlcek commented Jan 30, 2019

I'm going to squash all the commits.
@juliancheal Please take a look.

@jvlcek jvlcek force-pushed the bz_1645041_del_auto_domains branch from 45f2534 to a39a2f4 Compare January 30, 2019 17:56
@miq-bot
Copy link
Member

miq-bot commented Jan 30, 2019

Checked commit jvlcek@a39a2f4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/controllers/api/automate_domains_controller.rb

@mkanoor
Copy link
Contributor

mkanoor commented Jan 30, 2019

👍

@juliancheal
Copy link
Member

LGTM @jvlcek

@gtanzillo gtanzillo merged commit 1efd8d9 into ManageIQ:master Feb 1, 2019
@gtanzillo gtanzillo added this to the Sprint 104 Ending Feb 4, 2019 milestone Feb 1, 2019
@jvlcek jvlcek deleted the bz_1645041_del_auto_domains branch June 3, 2019 21:13
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.

None yet

5 participants