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 native_attr to OPTIONS of Tower Credentials #14902

Closed
wants to merge 1 commit into from

Conversation

jameswnl
Copy link
Contributor

As a proposed to solution to #14900

Part of bug https://bugzilla.redhat.com/show_bug.cgi?id=1444398

@miq-bot add_labels wip, bug, providers/ansible_tower

@miq-bot miq-bot changed the title Add native_attr to OPTIONS of Tower Credentials [WIP] Add native_attr to OPTIONS of Tower Credentials Apr 26, 2017
@miq-bot miq-bot changed the title [WIP] Add native_attr to OPTIONS of Tower Credentials Add native_attr to OPTIONS of Tower Credentials Apr 26, 2017
@miq-bot miq-bot removed the wip label Apr 26, 2017
@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2017

Checked commit jameswnl@e904d78 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

@jameswnl
Copy link
Contributor Author

@mzazrivec will this be helpful?

@mzazrivec
Copy link
Contributor

The problem with this approach is that it introduces inconsistency between what you
get from our rest api when you query credential OPTIONS and what you actually get
when you GET the actual credential resource.

This makes me wonder why we're even telling the world (via api) what info you can
get, when in truth you won't get it. What's the point really?

The situation with the credentials api is already bad as it is (since there's already inconsistency
between what you GET and what you need to POST for it to work) and this just makes
the bad situation worse.

We cannot reasonably expect the api consumers (angular UI or anything else) to understand
semantics of this native_attr (btw. you have typo in your PR: navtive_attr) and create
some obscure if statements for this to make it work.

So no from my end.

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