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 #197

Closed
wants to merge 1 commit into from

Conversation

jmartisk
Copy link
Contributor

@jmartisk jmartisk commented Jun 1, 2023

@gastaldi
Copy link
Collaborator

gastaldi commented Jun 1, 2023

Nice, Can we have a test for that endpoint too?

@jmartisk
Copy link
Contributor Author

jmartisk commented Jun 1, 2023

I've added a test, please check, not sure what else it would make sense to verify

Copy link
Collaborator

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

@aloubyansky
Copy link
Member

I'd prefer the API, it should be easy to create a config object and serialize it. We made it easy for the files but I think the mappers are exposed through the API too.

@maxandersen
Copy link

not following what is the point of this end point?

the generated content is unnecessary in 99% of all usecases is it not?

@aloubyansky
Copy link
Member

99% of all use-cases? :D

@aloubyansky
Copy link
Member

We could introduce an option to enable this endpoint. It helps automate QE processes.

@maxandersen
Copy link

Just trying to grok when it QE need to specify all these Settings?

@maxandersen
Copy link

Just to be explicit:

In what scenario is it we prefer the registry config is using absolute explicit overrides like setting the maven url over using the derived value (meaning we don't test the usecase 99% of users use - just setting the registry id) ?

And then if I read this pr right this code will generate using the host name the service itself thinks it is running on rather than the necessary the host name users access it with ? That is normally considered a bad thing security wise that a service does that as it reveals details on the internal infrastructure which can be used to create attacks.

That's the context of my question.

If there are a need to generate sample configs with this full kind of info isn't it better done with a api or cli command ?
And if really part of the service - have it use the actual user provided hostname setup?

@aloubyansky
Copy link
Member

The user in this case would QE. Our staging/QE registries actually never had hosts derivable from registry IDs. The logic deriving config from registry ID is pretty much unit tested. Until now we had a single staging registry, however it's not scaling when testing multiple builds of the same stream. So now for every build we spin up a pre-configured registry instance, the URL of which will be included in a UMB message QE receives and triggers testing automatically. Adding this endpoint will help fetch the proper config to configure the client tools. It doesn't have to be enabled on registries running in production.

@maxandersen
Copy link

Sure - but still; why not just have a command/code separate from the registry that qe and others can adjust to what it needs separate from what build of the registry is used?

@aloubyansky
Copy link
Member

Because it'd be probably the easiest way to get the config. Although not the only one.

@jmartisk jmartisk force-pushed the q3155 branch 2 times, most recently from f623df7 to 934b766 Compare June 14, 2023 07:55
@jmartisk
Copy link
Contributor Author

jmartisk commented Jun 14, 2023

Changes in my latest update:

  • Switched to YAML serializer instead of building a String manually
  • Only expose this new endpoint if quarkus.registry.expose.client.config.yaml=true (I had to move the REST method to a separate class to be able to do that...)

Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jmartisk

@gastaldi
Copy link
Collaborator

Superseded by #201

@gastaldi gastaldi closed this Jun 21, 2023
@jmartisk jmartisk deleted the q3155 branch June 21, 2023 14:49
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

4 participants