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

Upgrade forbiddenapis to 3.5.1 #6840

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

uschindler
Copy link
Contributor

Description

The new version was released a minute ago: https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-35-released-2023-03-27

Summary of relevant changes for Opensearch:

  • Faster startup time as Gradle plugin initialization was compiled statically and is part of JAR file
  • Support for Java 20 signatures and bytecode of Java 21

@uschindler
Copy link
Contributor Author

uschindler commented Mar 27, 2023

No idea why the checks failed. Please merge if you like, it is up to you. I won't add any more fixes to the PR.

This PR is of informational "theres something to upgrade after new release" purpose only. Do whatever you think is right. Thanks.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Mar 27, 2023

No idea why the checks failed. Please merge if you like, it is up to you. I won't add any more fixes to the PR.

@uschindler most of the Gradle tasks are failing with

> Third party audit task is not configured correctly

I am wondering have you run the precommit check locally (./gradlew precommit, just to understand if this is CI issue or plugin configuration indeed)? thank you

@uschindler
Copy link
Contributor Author

I only tried:

$ gradlew forbiddenApis

Passed.

I did not try precommit but it fails for whatever reason. I assume it is because of the new version changes some logging messages and the CLI parser falls on this. I have no time to look into this, as this third party audit task is some hack by @rmuir.

@uschindler
Copy link
Contributor Author

At moment Github is completely down, no merging of PRs works not adding comments. See Twitter...

@uschindler
Copy link
Contributor Author

This is caused by thirdPartyAudit: "Unnecessary exclusions, following classes are not missing"

This no longer works that way as the forbiddenapis checker has toned down warning messages, so the check may need to be disabled. I have no idea where it can be found...

@reta
Copy link
Collaborator

reta commented Mar 27, 2023

This is caused by thirdPartyAudit: "Unnecessary exclusions, following classes are not missing"

Thanks @uschindler

1 similar comment
@reta
Copy link
Collaborator

reta commented Mar 27, 2023

This is caused by thirdPartyAudit: "Unnecessary exclusions, following classes are not missing"

Thanks @uschindler

@uschindler
Copy link
Contributor Author

I think you cant do it like that anymore. The thirdPartyAuditChecker relies on regular expressions to find some missing classes. This does not work anymore, sorry.

Maybe scan those thirs party audits with the old CLI version. I do not even understand what the code is doing... @rmuir ?

@uschindler
Copy link
Contributor Author

This is caused by this change: policeman-tools/forbidden-apis#210

There's no workaround as all class names missing are no longer listed, so maybe the code is too strict, but I have no idea how to fix it - because I don't know why it complains at all.

@uschindler uschindler force-pushed the dev/forbiddenapis-3.5 branch from 767ea05 to 48041e5 Compare March 27, 2023 13:22
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Mar 28, 2023

@uschindler mind if I move this one under #6792 umbrella? will work on it as part of JDK-20 support story ... ty!

@uschindler
Copy link
Contributor Author

Ok. I the meantime I understood what the code wants to do.
The new version of forbiddenapus can't be used as is. I will release a new "Bugfix / minor" update that restores the detailed "class missing" messages if you run with debug logging: policeman-tools/forbidden-apis#226 (this issue also refers to the detailed analysis of the jar hell checker).

@reta
Copy link
Collaborator

reta commented Mar 28, 2023

Ok. I the meantime I understood what the code wants to do.

This is great, thank you!

@uschindler uschindler changed the title Upgrade forbiddenapis to 3.5 Upgrade forbiddenapis to 3.5.1 Mar 30, 2023
@uschindler
Copy link
Contributor Author

Hi,
version 3.5.1 was released to work around your problem. Basically adding the --debug option to the CLI script and adapting the regular expression a bit should be enough to restore previous behaviour.

https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-351-released-2023-03-30

@reta
Copy link
Collaborator

reta commented Mar 30, 2023

Hi, version 3.5.1 was released to work around your problem. Basically adding the --debug option to the CLI script and adapting the regular expression a bit should be enough to restore previous behaviour.

https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-351-released-2023-03-30

@uschindler do you intend to make the change in this pull request or I could accommodate the update in Gradle 8.1 migration pull request? thank you!

@uschindler
Copy link
Contributor Author

Hi, version 3.5.1 was released to work around your problem. Basically adding the --debug option to the CLI script and adapting the regular expression a bit should be enough to restore previous behaviour.
https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-351-released-2023-03-30

@uschindler do you intend to make the change in this pull request or I could accommodate the update in Gradle 8.1 migration pull request? thank you!

running tests....

Will soon update.

@uschindler
Copy link
Contributor Author

I pushed my changes. I also removed the dependency in the testKit. I have no idea how to test this... ../gradlew integTest in the buildSrc directory does not execute this test setup.

It would be good to verify if this works as expected.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.shrink/30_copy_settings/Copy settings during shrink index}

@reta
Copy link
Collaborator

reta commented Mar 30, 2023

@uschindler could you please rebase against latest main, the flaky tests should be gone, thank you

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>
Signed-off-by: Uwe Schindler <uwe@thetaphi.de>
@uschindler uschindler force-pushed the dev/forbiddenapis-3.5 branch from b7dbef2 to 772e9b8 Compare March 30, 2023 15:12
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta reta merged commit 37b37c5 into opensearch-project:main Mar 30, 2023
@reta reta added the backport 2.x Backport to 2.x branch label Mar 30, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6840-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 37b37c5b518c5bdb2dfdc5250117cfd39394edeb
# Push it to GitHub
git push --set-upstream origin backport/backport-6840-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6840-to-2.x.

@uschindler uschindler deleted the dev/forbiddenapis-3.5 branch March 30, 2023 15:52
reta pushed a commit to reta/OpenSearch that referenced this pull request Mar 30, 2023
* Upgrade forbiddenapis to 3.5

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

* Update to forbiddenapis 3.5.1 and change regex for missing classes

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

---------

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>
(cherry picked from commit 37b37c5)
reta added a commit that referenced this pull request Apr 1, 2023
* Upgrade forbiddenapis to 3.5

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

* Update to forbiddenapis 3.5.1 and change regex for missing classes

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

---------

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>
(cherry picked from commit 37b37c5)

Co-authored-by: Uwe Schindler <uwe@thetaphi.de>
mitrofmep pushed a commit to mitrofmep/OpenSearch that referenced this pull request Apr 5, 2023
* Upgrade forbiddenapis to 3.5

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

* Update to forbiddenapis 3.5.1 and change regex for missing classes

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>

---------

Signed-off-by: Uwe Schindler <uwe@thetaphi.de>
Signed-off-by: Valentin Mitrofanov <mitrofmep@gmail.com>
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.

2 participants