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.

Sign up for GitHub

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

Jump to bottom

provide no path for pulp3 api url #552

Merged
merged 1 commit into from Jan 2, 2020
Merged

provide no path for pulp3 api url #552

merged 1 commit into from Jan 2, 2020

Conversation

jlsherrill
Copy link
Contributor

No description provided.

@jlsherrill
Copy link
Contributor Author

katello expects this url to look like: https://hostname/pulp/

I'm not completely sold on that (but we can change katello), but i would think it should either be one of:

https://hostname
https://hostname/pulp/
https://hostname/pulp/api/v3/

i'm not sure it makes sense to be just 'https://hostname/pulp/api/'. I'm kinda partial to the 3rd one in the list (requiring an update here and in katello)

@jlsherrill
Copy link
Contributor Author

actually among further inspection, the client bindings themselves want 'https://hostname/pulp', so i propose we go with that.

@ekohl
Copy link
Member

ekohl commented Dec 17, 2019

For what it's worth, pulpcore now explicitly only binds /pulp/api/v3 (option 3) and it would be nice to get consistency on this. That would be a good reason to let Katello follow suit.
https://github.com/theforeman/puppet-pulpcore/blob/8788f02ef2715be79b0d13f37241b12b5f6aa075/manifests/apache.pp#L4

On the other hand, encoding the API version is not nice. Given the bindings expect /pulp, it'd be the cleanest.

@jlsherrill
Copy link
Contributor Author

actually i'm sorry, the bindings actually only care about the hostname and the scheme. It seems the path is ignored!

@jlsherrill
Copy link
Contributor Author

So i am good with the 3rd option as well, if we want to go that route

@jlsherrill jlsherrill changed the title exclude api from pulp3 api url use full v3 api path for pulp3 Dec 17, 2019
@jlsherrill
Copy link
Contributor Author

opened a katello pr to adjust: Katello/katello#8479

@jlsherrill
Copy link
Contributor Author

chatting with @ehelms he brought up a decent point. If the bindings are assuming /pulp/api/v3/ then maybe this should just provide the hostname and scheme. This would also be more future proof in case of a /v4 api. Thoughts?

@ekohl
Copy link
Member

ekohl commented Dec 18, 2019

If the client bindings ignore the path, I'd be fine with reducing the URL to just scheme + hostname (+ port).

@jlsherrill
Copy link
Contributor Author

This is now green (thanks to whoever fixed the broken tests in nightly!). This is needed to turn on pulp3 in nightly

@ekohl ekohl changed the title use full v3 api path for pulp3 provide no path for pulp3 api url Jan 2, 2020
ekohl
ekohl approved these changes Jan 2, 2020
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Since this is a default, this would normally need a migration in foreman-installer but these options were added only in nightly so it doesn't need one now.

@ekohl ekohl merged commit 5560a32 into theforeman:master Jan 2, 2020
@jlsherrill jlsherrill deleted the api branch January 2, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jlsherrill @ekohl @theforeman-bot