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

Allow applications using quakus-info to contribute data to the /info using CDI #38029

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

jtama
Copy link
Contributor

@jtama jtama commented Jan 4, 2024

@jtama
Copy link
Contributor Author

jtama commented Jan 4, 2024

@geoand

@geoand
Copy link
Contributor

geoand commented Jan 4, 2024

Thanks, I'll have a look tomorrow

@geoand
Copy link
Contributor

geoand commented Jan 4, 2024

So if I understand correctly you want to make all implementations of InfoContributor beans automatically?

I think it would be safer to have users opt-in to that by explicitly annotating their implementations with any of the standard CDI annotations.

@geoand
Copy link
Contributor

geoand commented Jan 4, 2024

Another issue I see is that classes under runtime are considered internal, so we would probably want to move InfoContributor

@jtama
Copy link
Contributor Author

jtama commented Jan 4, 2024

So if I understand correctly you want to make all implementations of InfoContributor beans automatically?

I think it would be safer to have users opt-in to that by explicitly annotating their implementations with any of the standard CDI annotations.

Ok I can remove the scan then, and just let user opt-in.

Another issue I see is that classes under runtime are considered internal, so we would probably want to move InfoContributor

Ok do you want me to move it to io.quarkus.info.spi ?

Also even If i did test the extension on a local project I can't find any integration-test for this extension AND

@geoand
Copy link
Contributor

geoand commented Jan 5, 2024

Ok do you want me to move it to io.quarkus.info.spi ?

I would say move them to io.quarkus.info as it is no longer an SPI :)

Also even If i did test the extension on a local project I can't find any integration-test for this extension

We have a bunch of tests in here

@jtama
Copy link
Contributor Author

jtama commented Jan 5, 2024

Ok do you want me to move it to io.quarkus.info.spi ?

I would say move them to io.quarkus.info as it is no longer an SPI :)

Also even If i did test the extension on a local project I can't find any integration-test for this extension

We have a bunch of tests in here

Ok in. this case shoulld I completely remove the runtime-spi module ?

@geoand
Copy link
Contributor

geoand commented Jan 5, 2024

Actually now that I think about it, let's just leave the class in the module and package that it already resides.
The package is not great, but it's not bad enough to break extensions that already use it

@jtama
Copy link
Contributor Author

jtama commented Jan 5, 2024

Ok so I have something working with opt-in. But that has a drawback.

As long as now one asks specificly to get those InfoContributor instances injected, they won't be created.

Soi for this to work I had to the following to the runtime module

  @ApplicationScoped
public class ExternalContributorReporter {

    @Inject
    Instance<InfoContributor> contributors;


    public void report(Map<String, Object> finalBuildInfo) {
        contributors.forEach(contributor -> finalBuildInfo.put(contributor.name(), contributor.data()));
    }
}

And use the reporter in the InfoRecorder class.

Not sure what we prefere.

@geoand
Copy link
Contributor

geoand commented Jan 5, 2024

As long as now one asks specificly to get those InfoContributor instances injected, they won't be created.

The extension can make the beans of type InfoContributor unremovable.
That will ensure that they are always added to the application

@jtama
Copy link
Contributor Author

jtama commented Jan 5, 2024

I think we have something nice now

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Looks great!

I just added a small comment worth addressing

@jtama
Copy link
Contributor Author

jtama commented Jan 5, 2024

Looks great!

I just added a small comment worth addressing

Stream removed

@geoand geoand changed the title Allow applications using quakus-info to contribute data to the /info at rutime Allow applications using quakus-info to contribute data to the /info using CDI Jan 5, 2024

This comment has been minimized.

@jtama
Copy link
Contributor Author

jtama commented Jan 5, 2024

Fixed format.

@geoand
Copy link
Contributor

geoand commented Jan 5, 2024

Can you please squash the commits?

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 5, 2024

This comment has been minimized.

Copy link

github-actions bot commented Jan 5, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

This comment has been minimized.

@gsmet gsmet merged commit 32569ff into quarkusio:main Jan 8, 2024
22 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 8, 2024
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Jan 8, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Provide a way for applications to contribute to the /info enpoint
4 participants