Skip to content

Conversation

@prdoyle
Copy link
Contributor

@prdoyle prdoyle commented Feb 18, 2025

See ES-10905.

@prdoyle prdoyle added >non-issue :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged test-entitlements v8.18.1 v8.19.0 v9.0.1 v9.1.0 labels Feb 18, 2025
@prdoyle prdoyle self-assigned this Feb 18, 2025
@prdoyle prdoyle requested a review from a team as a code owner February 18, 2025 23:18
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 18, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@prdoyle prdoyle added :Core/Infra/Entitlements Entitlements infrastructure and removed :Core/Infra/Core Core issues without another label labels Feb 18, 2025
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks fine, a couple nits

var instrumentationMethod = checkMethods.get(key);
if (instrumentationMethod != null) {
// System.out.println("Will instrument method " + key);
logger.debug("Will instrument {}", key);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please split this out? We've talked about it across several PRs, let's do it, but as distinct commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the controversy was using exceptions to get a stack trace.

Copy link
Member

Choose a reason for hiding this comment

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

There's no controversy, I just think adding debug logging to instrumentation is unrelated to adding miscellaneous file entitlements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

@EntitlementTest(expectedAccess = PLUGINS)
static void createScannerFile() throws FileNotFoundException {
Copy link
Member

Choose a reason for hiding this comment

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

Aren't all of the rest of the methods in this file also in java.base? I don't understand the distinction as to why they belong in a separate files vs in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't check. I just moved over the ones that were on the spreadsheet tab.

What would you like to do? Merge them together?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe split them based on some explainable attributes? I don't feel strongly, but it seems like having a test file that implies java.base classes should be tested in it, yet other test files contain java.base classes, will cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I can explain why these ended up in their own spreadsheet tab. Maybe I'll just merge them into FileCheckActions.

}

@EntitlementTest(expectedAccess = PLUGINS)
static void zipFile_1() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

In other tests we've named these logically based on the parameters. Could we do that here too?

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@prdoyle prdoyle merged commit 877963c into elastic:main Feb 19, 2025
22 checks passed
@prdoyle prdoyle deleted the java.base branch February 19, 2025 18:17
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

prdoyle added a commit to prdoyle/elasticsearch that referenced this pull request Feb 19, 2025
* java.base entitlements

* SuppressForbidden, and add a missing test

* Revert logging back to commented-out printlns

* Merge FileCheckActions and rename for overloads

* Remove stray logger

* Remove more traces of logging change

* Remove more traces of logging
prdoyle added a commit to prdoyle/elasticsearch that referenced this pull request Feb 19, 2025
* java.base entitlements

* SuppressForbidden, and add a missing test

* Revert logging back to commented-out printlns

* Merge FileCheckActions and rename for overloads

* Remove stray logger

* Remove more traces of logging change

* Remove more traces of logging
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
* java.base entitlements

* SuppressForbidden, and add a missing test

* Revert logging back to commented-out printlns

* Merge FileCheckActions and rename for overloads

* Remove stray logger

* Remove more traces of logging change

* Remove more traces of logging
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
* java.base entitlements

* SuppressForbidden, and add a missing test

* Revert logging back to commented-out printlns

* Merge FileCheckActions and rename for overloads

* Remove stray logger

* Remove more traces of logging change

* Remove more traces of logging
elasticsearchmachine pushed a commit that referenced this pull request Feb 19, 2025
* java.base entitlements

* SuppressForbidden, and add a missing test

* Revert logging back to commented-out printlns

* Merge FileCheckActions and rename for overloads

* Remove stray logger

* Remove more traces of logging change

* Remove more traces of logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants