Skip to content

Conversation

holly-cummins
Copy link
Contributor

@holly-cummins holly-cummins commented Jun 25, 2025

See discussion here: #48432 (comment)

In order to make the ban work, I explicitly list every logger method, so that Log itself didn't trigger the ban. An alternative would be to had to exclude Logger itself from forbidden api checking, either with a maven exclude or @SuppressForbidden annotation. That reduces the risk of missing one method from my list, but increases the risk of missing something else banned in Log.

While getting CI green I spotted three classes which were using Log. Only one of them was my fault, as far as I can tell. :)

In case we want it later, this is what we maven exclude looks like:

                                <!-- We want to exclude Log from the check for Log or the checker goes mad, which means we have to exclude it from all checks -->
                                <excludes>
                                    <exclude>io/quarkus/logging/Log.class</exclude>
                                </excludes>

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file labels Jun 25, 2025
@holly-cummins holly-cummins requested review from geoand and gsmet June 25, 2025 17:06

This comment has been minimized.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I think this is good and we should merge it but I would like to have #48655 in first as atm ForbiddenAPIs is run on a small subpart of the tree so there might be more changes needed here once we generalize it.

I also think once my PR is in, we should start aggressively ban everything that shouldn't be used in our tree.

gsmet and others added 2 commits July 3, 2025 18:32
At some point, we will provide specific rules but for now, let's be
lenient.
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I rebased on top of my PR and this should be all good now.

It spotted several more occurrences so it was worth applying the plugin to the whole codebase.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 3, 2025

This comment has been minimized.

Copy link

quarkus-bot bot commented Jul 4, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 67e525c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@holly-cummins holly-cummins merged commit aa054b1 into quarkusio:main Jul 4, 2025
111 of 112 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.25 - main milestone Jul 4, 2025
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants