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

Detect breaking changes on pull requests v2.0 #12974

Merged

Conversation

peternied
Copy link
Member

@peternied peternied commented Mar 28, 2024

Description

Adds a gradle task and github action to check for breaking changes made to the APIs in server by running comparison against most resent snapshot build on sonotype maven repository.

Only classes with @PublicApi are checked for breaking changes.

Uses japicmp to perform the comparison against the jar files, learn more https://siom79.github.io/japicmp/

Example

Workflow failure where a plugin method was removed [link]

* What went wrong:
Execution failed for task ':server:japicmp'.
> A failure occurred while executing me.champeau.gradle.japicmp.JApiCmpWorkAction
   > Detected binary changes.
         - current: opensearch-3.0.0-SNAPSHOT.jar
         - baseline: opensearch-3.0.0-SNAPSHOT.jar.
     
     See failure report at file:///home/runner/work/OpenSearch-1/OpenSearch-1/server/build/reports/java-compatibility/report.html

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Get more help at https://help.gradle.org./

BUILD FAILED in 2m 18s
33 actionable tasks: 33 executed
Error: Gradle build failed: see console output for details
Comparing source compatibility of  against 
***! MODIFIED CLASS: PUBLIC ABSTRACT org.opensearch.plugins.Plugin  (not serializable)
	===  CLASS FILE FORMAT VERSION: 55.0 <- 55.0
	---! REMOVED METHOD: PUBLIC(-) java.util.Collection<org.opensearch.common.inject.Module> createGuiceModules()

Related Issues

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Adds a gradle task and github action to check for breaking changes made
to the APIs in server by running comparison against most resent snapshot
build on sonotype maven repository.

Only classes with @publicapi are checked for breaking changes.

Uses japicmp to perform the comparison against the jar files, learn
more https://siom79.github.io/japicmp/

Signed-off-by: Peter Nied <[email protected]>
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 6ce041e

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/performance-analyzer.git]

Copy link
Contributor

✅ Gradle check result for 6ce041e: SUCCESS

@reta
Copy link
Collaborator

reta commented Mar 29, 2024

Do you have an example of the report in case of breaking change, just to share on pull request description with everyone, thank you

@peternied
Copy link
Member Author

Do you have an example of the report in case of breaking change, just to share on pull request description with everyone, thank you

@reta Good idea as always! I've updated the description to include a link to an example along side the output of a failure.

@peternied
Copy link
Member Author

@bbarani @gaiksaya @andrross @kotwanikunal Any thoughts on this change before we start enforcing it?

@peternied peternied added the backport 2.x Backport to 2.x branch label Mar 29, 2024
@andrross
Copy link
Member

@peternied Looks reasonable to me.

@bbarani
Copy link
Member

bbarani commented Mar 29, 2024

+1 Looks good to me.

Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

@peternied For in between major versions making intentional breaking change to this APIs I believe this check would fail. Would we hard merge then?

.github/workflows/detect-breaking-change.yml Show resolved Hide resolved
Copy link
Member

@kotwanikunal kotwanikunal left a comment

Choose a reason for hiding this comment

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

LGTM.

@peternied
Copy link
Member Author

@peternied For in between major versions making intentional breaking change to this APIs I believe this check would fail. Would we hard merge then?

@gaiksaya Public APIs are promises to our development partners of how our software works - it should be hard to break these promises. There will be good reasons to break them including security vulnerabilities, useful features [1], or to remove unused parts of the product [2]. Pull request check failures are a good signal - maintainers can still bypass and merge, but we can evaulate over time if we want a stronger enforcement or more formalized workaround process in place.

@peternied peternied merged commit 2dc071f into opensearch-project:main Apr 2, 2024
116 of 118 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 2, 2024
(cherry picked from commit 2dc071f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
srikanthpadakanti pushed a commit to srikanthpadakanti/OpenSearch that referenced this pull request Apr 2, 2024
reta pushed a commit that referenced this pull request Apr 2, 2024
(cherry picked from commit 2dc071f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Peter Nied <[email protected]>
@peternied peternied deleted the breaking-change-detecter branch April 2, 2024 15:36
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
harshavamsi pushed a commit to harshavamsi/OpenSearch that referenced this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Issues with external APIs backport 2.x Backport to 2.x branch bug Something isn't working enhancement Enhancement or improvement to existing feature or request Plugins
Projects
None yet
7 participants