-
Notifications
You must be signed in to change notification settings - Fork 898
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
Re-Add API_OPTIONS to EmbeddedAnsible credentials #18854
Re-Add API_OPTIONS to EmbeddedAnsible credentials #18854
Conversation
Welp... I guess I have to align some hashes... "burb"... |
I think you can stick with WIP and continue with getting credentials CRUD in place unless having these makes some significant improvement in the meantime. |
@carbonin okay. I was considering just trying to push things into master smaller at a faster rate, but I can get behind that workflow as well. |
Removed as part of the ansible-runner effort, but added back manually now (instead of inheriting from `manageiq-providers-ansible_tower`). This only allows the UI to display and insert credentials, but it does not implement them when using runner.
f1d5745
to
7dfbba5
Compare
Checked commit NickLaMuro@7dfbba5 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
What does "This only allows the UI to display and insert credentials" and "it seems like adding a credential will make the API request, but the worker doesn't seem to process anything" mean together? What is the actual end result of this PR? I was assuming that a new credential added after this change still wouldn't get displayed in the UI ... right? Maybe if we make this PR create the record in |
So the UI actually does a fetch to to basically generate the forms dynamically from it (updated the PR description, so see above). This basically just loops over the manageiq/app/models/authentication.rb Lines 136 to 142 in f812391
And if you want to see what they look like, you can take a look at this gist I put together: https://gist.github.com/NickLaMuro/bcd6a14652e74ca47abeba7d65d4b4cb (sorry the pics are zoomed in...) But as you can see, they are dynamically generated, but are all there with just these changes.
So what is weird about this is, as shown in the gist above, you can enter credentials just fine: And it even displays as if it was sucessfull: You end up with a result of: {"results":[{"success":false,"message":"type not currently supported"}]} (and that is a So to the meat of your questions:
This basically "un-breaks" the UI display issues, but doesn't currently implement them (it seems). I knew as part of pushing #18854 that we didn't have credentials functioning that I would be circling back as my next effort. And what I am doing here is simply taking a carbon copy of what is in
and re-introducing it back here (minus the I plan on doing a PR for each of these individually from here on out to make sure they then work with That said, getting them just to save might be worth doing that here since that should apply to all of the record types... but I think this a lot to digest as it is, so let me know what you think (hopefully this helps). |
Thanks for the explanation @NickLaMuro that makes sense to me. In that case I agree that we can merge this now because it would allow us to model and deal with the different credential types one by one or simultaneously. Additionally I think this may un-break the UI specs. |
Confirming this fixes the travis failure! 👍 (And the ansible credentials textual summary screen shows
(all empty, but present and no error). Thanks! 👍 |
Removed as part of the ansible-runner effort, but added back manually now (instead of inheriting from
manageiq-providers-ansible_tower
).This only allows the UI to display and insert credentials, but it does not implement them when using runner.
Unsure if this deservse a
[WIP]
label. When testing this on an appliance, it seems like adding a credential will make the API request, but the worker doesn't seem to process anything and produce a result. Unsure if this deserves to be fixed here or we can fix in a later PR.Before
After
Steps for Testing/QA
To confirm this now works, with this code in place:
But, as mentioned above, doesn't seem to create records anywhere so unsure if we should be merging this as is or fix other things as well before doing so.