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 #9044

Closed

Conversation

peternied
Copy link
Member

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.

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

Related Issues

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

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

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

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

Copy link
Collaborator

@nknize nknize 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 premature. We should address #8127 first and provide a mechanism by which we can identify specific classes that provide BWC guarantees vs those that don't and use the annotation to enforce. For now we only have java annotations (which aren't enforceable) but at least they provide a very clear definition of what does and does not provide BWC guarantees.

Similarly, any class explicitly marked with the @opensearch.internal annotation, or not explicitly marked by an annotation should not be extended by external implementation components as it does not guarantee backwards compatibility and may change at any time.

Adding a carte blanche check to fail on build classpath is gatekeeping the codebase that Elastic never intended to have as a public facing ecosystem.

Let's converge on defining the public facing ecosystem before unilaterally breaking that progress.

@peternied
Copy link
Member Author

peternied commented Aug 1, 2023

I think this is premature.

@nknize 100% disagree - changing the goal post on the problem by slicing/dicing what is 'public' or not does not address the problem that there are compile time relationships that are broken on a weekly basis in downstream libraries.

@nknize
Copy link
Collaborator

nknize commented Aug 1, 2023

If OpenSearch is exposing classes that should not be consumed those classes should be isolated so plugins cannot use them.

What do you think we're trying to do? This PR stops that.

I get it.. downstreams are frustrated. The reality is that OpenSearch forked Elasticsearch which was NEVER designed as an "ecosystem" thus package modifiers was never a design tenet. I worked there for six years. Lucene had a hard enough time getting to modularity as well and that has a well defined public API. We can have mechanisms to help absorb the pain, but this is not at all productive. This is destructive.

Have a read of #5910 and let me know what questions you have.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator

nknize commented Aug 1, 2023

I think #8486 will help with this a lot. Specifically for me it will help prioritize backporting PRs and better surfacing downstreams that are broken. I'm happy to help any of those maintainers that are struggling to change imports or add a new dependency to the build.grade.

@peternied
Copy link
Member Author

We can have mechanisms to help absorb the pain, but this is not at all productive. This is destructive.

OpenSearch does not have any required checks to merge. The only mechanism that gates merging is one codeowner (maintainer) approval. Adding this check informs maintainers/contributors if there will be a breaking API change.

A check that can be ignored does not seem destructive, no?

@Yury-Fridlyand
Copy link

Is there a way to get subscribed/notified about a breaking change with new task?
If not - is anything planned for future?

@peternied
Copy link
Member Author

#8486 is good and it is not enough

  • Plugins need to buildable for the check to helpful.
  • There are plugins not inside the OpenSearch-Project that do not get this level of support.

I think we can merge both changes. Once is focused on the specific impact of changes (nice!) and this change is focused around if there could be any downstream impact or not.

@reta
Copy link
Collaborator

reta commented Aug 1, 2023

A check that can be ignored does not seem destructive, no?

I think the question is - what is the goal of this check if it is always would be a noise in the build? I think about checks as manifests of the problem and signs the things should not be merged

@peternied
Copy link
Member Author

I think about checks as manifests of the problem and signs the things should not be merged

If every time we had a justification for that failures shouldn't block merging - I'd feel pretty good about that. Today we don't have the insight and instead rely on intentions of reviews to prompt why an public API breaking change was made.

@reta
Copy link
Collaborator

reta commented Aug 1, 2023

Today we don't have the insight and instead rely on intentions of reviews to prompt why an public API breaking change was made.

This is not true, we do have the insights (knowing ahead of time with 100% chances things will break), for the reasons @nknize pointed out (discussion in issues) the changes were carried forward. Don't get me wrong - I do want to shield every single dependent project from changes in core, but the legacy we have sets hard trade offs.

@nknize
Copy link
Collaborator

nknize commented Aug 2, 2023

Is there a way to get subscribed/notified about a breaking change with new task?

If we could create teams this would absolutely be doable. We'd just have the "Autobots engage" the downstream team through an @ comment. I think one alternative we have would be more issue spam? Another would be to have a slack bot post the failure to a build channel? Although @gaiksaya compatibility GHA already surfaced eight repos that seem to have either ditched the backport branching strategy (for shame!) or fail for other reasons so I'd probably prefer a #{plugin}-build channel for each repo to avoid a single repo getting fail flooded. (kickban offense for ye "old" IRC folks 🙂)

Copy link
Member Author

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Taking a step back, how could we alter this pull request to be mergeable? I believe the core problem - detecting breaking changes is valuable to both consumers of OpenSearch, assuming this is the case, I think there are some areas where there is discussion to be had as concerns have been raised:

Are there other areas that are missed that are still outstanding? For the core maintainers, how flexible can we be on these areas?

I'm trying for an minimally viable change while I keep executing on my deliverables - I understand we can do more outside of OpenSearch for notifications, but I'd suggest we break those out onto the discussion onto the issue or create other issues for tracking those great ideas.

@reta
Copy link
Collaborator

reta commented Aug 2, 2023

Thanks @peternied , from my point of view

Are there other areas that are missed that are still outstanding?

We need to find the time to kick off work on #8127, that would help us to answer the question "what is the OS public API" and the consequence of that, detect the breaking changes promptly with the hard checks.

For the core maintainers, how flexible can we be on these areas?

The target to have JPMS supported by OpenSearch as it should leaves as no options as to continue with breaking changes like the ones we have seen lately. The compatibility check is good start but it is already leading to confusion (the ecosystem is not in the state where it should be), at this moment it affects all pull request and is not anyhow related to the their changes (in 99.9% cases).

@nknize
Copy link
Collaborator

nknize commented Aug 2, 2023

We need to find the time to kick off work on #8127, that would help us to answer the question "what is the OS public API"

💯 agree. #8486 already gives us a good mechanism, and it's first usage in #9053 (a two line non-breaking change) already detected eight downstreams that have deviated from main. Merging this PR, which fails the task on modification, will just create more noise while we work towards the correct end of creating a semver enforced public API.

Good intent here. Wrong mechanism at this point in time.

@nknize
Copy link
Collaborator

nknize commented Aug 2, 2023

Maybe we shelve this PR? I could see this check being valuable after we define the public surface area (e.g., post #5910 JPMS and #8127) so I'd hate to just close it and forget about it. Maybe we need to create a label, call it on-ice or future implementation or something, that identify contributions that are just a little pre-mature?

@peternied
Copy link
Member Author

@reta you mentioned wanting to take on making annotations for the project as part of #8127 [1], according to the documentation for japicmp annotations can be used as an inclusive or exclusive criteria. Do you think that change the utility of this PR if we had those annotations?

annotationIncludes List of annotations to include. The string must begin with '@'. Type: List

From the japicmp-gradle-plugin config

@reta
Copy link
Collaborator

reta commented Aug 2, 2023

Do you think that change the utility of this PR if we had those annotations?

Totally, japicmp is very powerful tool and we may end up using it. But the problem is two fold:

  • identify leaking APIs (the API which are not marked as public fe but that do leak into the public scope)
  • identify breaking changes (japicmp could be used there I think)

@dblock
Copy link
Member

dblock commented Aug 3, 2023

I think I agree that until core exposes an actual semver-compliant api, any attempt at highlighting breaking changes in PRs will be noise. I'd love to see a place where these changes are visible though in aggregate - maybe a cron job that lists and publishes all the breaking changes in any of the released versions of OpenSearch + SNAPSHOTs?

@peternied
Copy link
Member Author

peternied commented Aug 3, 2023

core exposes an actual semver-compliant api,

I like it, its one proposals in #8982 - I think it is better alternative to making pull requests checks noisy. It is unfortunate I can not commit more cycles to a semver focused effort.

@peternied
Copy link
Member Author

I'm am going to close out this PR and we can circle back or pick another direction with #9305

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.

5 participants