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

Make Authorizer pluggable #8090

Merged
merged 3 commits into from Feb 21, 2024
Merged

Make Authorizer pluggable #8090

merged 3 commits into from Feb 21, 2024

Conversation

snazy
Copy link
Member

@snazy snazy commented Feb 20, 2024

No description provided.

@snazy snazy requested a review from dimas-b February 20, 2024 14:23
dimas-b
dimas-b previously approved these changes Feb 20, 2024
}

public void eagerAuthorizerInitialization(
@Observes StartupEvent event, @SuppressWarnings("unused") Authorizer authorizer) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we actually need the Authorizer parameter? This being an instance method requires QuarkusAuthorizer to be created anyway, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly the intent here

Copy link
Member Author

Choose a reason for hiding this comment

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

It shall initialize it to run into possible config issues early

Copy link
Member

@dimas-b dimas-b Feb 20, 2024

Choose a reason for hiding this comment

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

That initialization will happen in the QuarkusAuthorizer constructor anyway. The Authorizer reference here will probably cause a CDI resolution error if there are more than one instance available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the class be annotated with @Startup instead?

https://quarkus.io/version/main/guides/lifecycle#startup_annotation

@snazy snazy merged commit 97e0dc7 into projectnessie:main Feb 21, 2024
17 checks passed
@snazy snazy deleted the authz-pluggable branch February 21, 2024 16:22
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