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

Expose an example config.yaml over the registry's REST api #201

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Conversation

gastaldi
Copy link
Collaborator

@jmartisk I have played with it and I don't think we need to introduce a new property to expose this configuration. IMHO it doesn't do any harm if it is always exposed.

I have refactored the tests a bit and used the model classes instead of relying on the String output. Let me know what you think about it

jmartisk and others added 2 commits June 20, 2023 12:35
This also moves the new endpoint to `DatabaseRegistryClient` since I don't think there is a need to introduce a new property to expose this
endpoint
@gastaldi gastaldi requested a review from jmartisk June 20, 2023 16:24
@jmartisk
Copy link
Contributor

AFAIR @aloubyansky asked for this not to be exposed in production deployments, so let's hear his opinion.

@aloubyansky
Copy link
Member

One of the concerns that @maxandersen raised in Jan's PR was that it reveals details of the internal infrastructure, which isn't desired in prod.

@gastaldi
Copy link
Collaborator Author

AFAIK the URL returned in the YAML is the same used to access it and it's configured via QUARKUS_REGISTRY_MAVEN_REPO_URL environment. Can you elaborate?

@gastaldi
Copy link
Collaborator Author

The same URL is also returned when the registry descriptor artifact is requested: https://github.com/quarkusio/registry.quarkus.io/blob/main/src/main/java/io/quarkus/registry/app/maven/RegistryDescriptorContentProvider.java#L93

@aloubyansky
Copy link
Member

In that case, it's fine, thanks for checking.
A possible concern could be we expose an endpoint w/o a requirement in production. Somebody may discover it and start relying it, while we may for some reason change or remove it.

@gastaldi
Copy link
Collaborator Author

gastaldi commented Jun 21, 2023

A possible concern could be we expose an endpoint w/o a requirement in production. Somebody may discover it and start relying it, while we may for some reason change or remove it.

I don't think that's the case here, we're just presenting a valid client configuration for the requested registry :)

Also, if that's the case, we don't necessarily need to expose it in the OpenAPI descriptor

@gastaldi gastaldi merged commit 8b56e1b into main Jun 21, 2023
1 check passed
@gastaldi gastaldi deleted the q3155 branch June 21, 2023 13:57
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.

None yet

3 participants