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

[WIP] Look for galaxy API info from galaxy_server.url #61927

Open
wants to merge 1 commit into
base: devel
from

Conversation

@alikins
Copy link
Contributor

commented Sep 6, 2019

For galaxy.ansible.com, the url configured in ansible.conf
should point to the base path of the galaxy api now.
For galaxy.ansible.com, that means:

[galaxy_server.release_galaxy]
url=https://galaxy.ansible.com/api/
token=my_token

So when looking for api 'available_versions', get
https://galaxy.ansible.com/api/ and it's contents
will have 'available_versions'

For other endpoint that may start the api in a different
deeper path, the url config will be

[galaxy_server.example]
url=https://galaxystuff.example.com/api/ansible/blip/

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@alikins alikins requested a review from jborean93 Sep 6, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

What’s the reason for this change? I’m not a fan of having the user specify the API base as it’s another bit of knowledge they need to know which they don’t need today. I understand that automation hub might have a different root than just /api but we already have fallback code specifically for that so why don’t we add it there. Is it meant to be customisable on the server?

@ansibot ansibot removed the needs_triage label Sep 6, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Also this seems like it would break existing usages of --server On the command line as now they have to specify the api path.

Look for galaxy API info from galaxy_server.url
For galaxy.ansible.com, the url configured in ansible.conf
should point to the base path of the galaxy api now.
For galaxy.ansible.com, that means:

[galaxy_server.release_galaxy]
url=https://galaxy.ansible.com/api/
token=my_token

So when looking for api 'available_versions', get
https://galaxy.ansible.com/api/ and it's contents
will have 'available_versions'

For other endpoint that may start the api in a different
deeper path, the url config will be

[galaxy_server.example]
url=https://galaxystuff.example.com/api/ansible/blip/

Fixes: #62073
@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Ping @alikins about my comments above. Maybe we should do something like this instead

  • Allow server URLs with and without the API path
  • If the path is not specified do:
    • Check {server}/api for a list of supported APIs
    • If a 404 then change to {server}/automationhub/api (or whatever AH is) and try again
    • If that is also a 404 then fail
  • If path is specified:
    • Check {server} for a list of supported APIs
    • If a 404 then fail

Basically we still have a way to override that fallback check for efficiency or if someone is doing something completely crazy on their end but for people who just want it to work then they only need to provide the scheme and hostname.

Even so we cannot just force a user to specify the api path as that would be a change in behaviour in a released version. So at a minimum we need to automatically add /api if no path is specified and require AH users to specify the path but I don't really want to force that as well.

@abadger abadger changed the title Look for galaxy API info from galaxy_server.url [WIP] Look for galaxy API info from galaxy_server.url Sep 11, 2019

@abadger abadger added the P2 label Sep 11, 2019

@abadger

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

Marking this as WIP and a potential P2. Note that this may not block release as I don't have enough information of what the final form looks like yet. However, I think that P2 is appropriate to make sure we re-evaluate this before rc1.

@abadger abadger added this to To do in 2.9 post beta firedrills via automation Sep 11, 2019

@ansibot ansibot added the WIP label Sep 11, 2019

@alikins

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@jborean93

What’s the reason for this change?

Nothing seems to work without it. I haven't tried the failback code.

I’m not a fan of having the user specify the API base as it’s another bit of knowledge they need to know which they don’t need today. I understand that automation hub might have a different root than just /api but we already have fallback code specifically for that so why don’t we add it there.

Which code is that? The bits in g_connect() decorator?

For this case, I'm not sure what we would failback to if the /api/ isn't correct. We could hardcode '/api/automation-hub/' as an option, but i don't know how set in stone that path is. For the automation-hub case, it is complicated slightly by the fact that '%{server_hostname}/api/' will be a valid URL (it won't 404) but it will not be the info expected.

Is it meant to be customisable on the server?

It is customisable in galaxy-api, though for the production 'cloud.redhat.com' path presumably won't change once it's live.

However, the path the /api/ could likely need to be changed for private galaxy installs.

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Which code is that? The bits in g_connect() decorator?

Yes, we currently try /api then retry /api with Bearer token auth already so we already have a fallback mechanism for detecting AH. My point was just fix up the fallback mechanism to work with AH in whatever case that may be.

As for the actual path not set in stone, why are we making changes to ansible-galaxy when the server side implement is not finalised. That should be sorted and figured out before doing your client work as we will just continue to play endless whack-a-mole as things change.

It is customisable in galaxy-api, though for the production 'cloud.redhat.com' path presumably won't change once it's live.

Ok so allow 2 scenarios

  1. The user specified just the scheme and hostname https://galaxy.ansible.com or https://cloud.redhat.com (or whatever AH is)
    • This provides a nice easy path for people, they don't need to know to put /api for Galaxy and /<insert whatever> for AH
    • Keeps AH behaving like Galaxy does making the transition from Galaxy to AH nice for users of Ansible
    • The barrier for entry for Red Hat/Ansible products is small, you just need to know the hostname
  2. The user has done something different on their endpoint and changed the path from /api to their own custom thing
    • We can't do anything about this and the user will have to specify the api root in their URL
    • This is the scenario where I expect a PR like this would be useful as it gives people the ability to use a custom path, they just need to document that in their own internal processes
    • There's nothing that stops them from doing the same for the Ansible/Red Hat Galaxy/AH instances but it shouldn't be mandatory
    • Even better, we are only forcing someone to specify the path once they exit the known universe of Galaxy/AH

At a minimum we still need to keep supporting 1 as existing releases for ansible-galaxy rely on that behaviour with things like ansible-galaxy role install --server .... Making the change to drop the automatic /api appending will break existing workflows and is not what we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.