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 #6647,BZ1120335 - Remove old foreman org destroy routes #4463

Merged
merged 1 commit into from
Jul 25, 2014
Merged

Fixes #6647,BZ1120335 - Remove old foreman org destroy routes #4463

merged 1 commit into from
Jul 25, 2014

Conversation

daviddavis
Copy link
Contributor

No description provided.

@daviddavis daviddavis changed the title [DO NOT REVIEW] Fixes #6647,BZ1120335 - Remove old foreman org destroy route Fixes #6647,BZ1120335 - Remove old foreman org destroy route Jul 21, 2014
@daviddavis daviddavis changed the title Fixes #6647,BZ1120335 - Remove old foreman org destroy route [DO NOT REVIEW] Fixes #6647,BZ1120335 - Remove old foreman org destroy route Jul 21, 2014
@daviddavis daviddavis changed the title [DO NOT REVIEW] Fixes #6647,BZ1120335 - Remove old foreman org destroy route Fixes #6647,BZ1120335 - Remove old foreman org destroy route Jul 21, 2014
@daviddavis daviddavis changed the title Fixes #6647,BZ1120335 - Remove old foreman org destroy route Fixes #6647,BZ1120335 - Remove old foreman org destroy routes Jul 21, 2014
@ehelms
Copy link
Member

ehelms commented Jul 21, 2014

@daviddavis if I read this correctly, the normal Foreman routes will now raise 404s?

@daviddavis
Copy link
Contributor Author

correct.

@ehelms
Copy link
Member

ehelms commented Jul 22, 2014

@daviddavis I am not sure if I like the idea of removing routes to replace them with a different path all together that's designed to do the same thing - seems confusing as a user to know to use /katello/api/v2/organizations for some actions and /api/v2/organizations for others. I'd maybe open this up to foreman-dev list to see what others opinions are.

@daviddavis
Copy link
Contributor Author

@ehelms how is this replacing paths with another path? AFAIK, that predates this PR.

@ehelms
Copy link
Member

ehelms commented Jul 22, 2014

@daviddavis what is 'that' in 'that predates' ? Are you not saying to users if you want to delete an organization via the API when Katello is present you have to visit a route that is not the normal API route for destroying an organization?

@daviddavis
Copy link
Contributor Author

According to the V2 apidoc, there are no /api/v2/organizations routes, only /katello/api/v2/organizations ones.

@daviddavis
Copy link
Contributor Author

Also, it may help to look at redmine again because I've updated/added issues. I've created three separate issues:

  1. Prevent users from bypassing dynflow delete via the old foreman routes (this issue)
  2. Address the organizations v2 api
  3. Address the v1 api

@ehelms
Copy link
Member

ehelms commented Jul 22, 2014

@daviddavis ahh right, the flaw in apipie that overrides routes. The other routes will all still be present though correct? So we'd need to remove all the original organization routes in favor of the Katello ones to prevent users from doing something bad? The issue with Apipie as well as how to handle this type of overriding in general for plugins seems important enough to warrant a mailing list discussion.

@daviddavis
Copy link
Contributor Author

I think those are good questions but I think they're more related to red mine issues 6708 and 6707. The purpose of this fix is to prevent users from deleting an org without dynflow.

@ehelms
Copy link
Member

ehelms commented Jul 22, 2014

Sorry, I guess I don't see the point in just preventing users from hurting themselves but leaving things in a weird state instead of just fixing the overall issue. What about the other organization routes that exist but are undocumented because they are overriden by our organizations controller?

@daviddavis
Copy link
Contributor Author

Assuming you're referring to the V2 api, doesn't this weird state currently exist? It's not my intention to address that in this PR. I think 6708 should fix that and I think it should fix routes that are undocumented but overriden by our organizations controller. If you're still hung up on the V2 change here, I suppose I can remove it from this PR and just let 6708 solve it.

@ehelms
Copy link
Member

ehelms commented Jul 22, 2014

I can be OK with disabling routes for now, but should we not simultaneously disable all the overridden routes? Looks like the organizations V2 controller in Katello has all the CRUD actions.

Removing old foreman destroy routes since they don't use dynflow to delete
organizations.
@daviddavis
Copy link
Contributor Author

@ehelms updated.

@ehelms
Copy link
Member

ehelms commented Jul 24, 2014

ACK

daviddavis pushed a commit that referenced this pull request Jul 25, 2014
Fixes #6647,BZ1120335 - Remove old foreman org destroy routes
@daviddavis daviddavis merged commit 9919e0b into Katello:master Jul 25, 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
Development

Successfully merging this pull request may close these issues.

2 participants