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

Incremental validation #187

Open
diareuse opened this issue Feb 5, 2024 · 2 comments
Open

Incremental validation #187

diareuse opened this issue Feb 5, 2024 · 2 comments

Comments

@diareuse
Copy link

diareuse commented Feb 5, 2024

The Problem

Most of the time the developer wants to know whether the changes done to the project are compatible with the previous version. This is not currently possible due to the hard fail during the apiCheck (check) phase. It verifies that the API has not changed at all, which loads the developer with more chores and responsibilities, that could be inherently automated.

Suggested solution

Introduce a mode in which compatibility validator actually only validates the binary compatibility (not identity) to the previous version. These changes almost exclusively include additions - new methods, new fields, new classes.

Since the compatibility validator already conveniently provides a diff of lines changed, new mode can be introduced to allow searching for removals and in only that case, hard fail the task.


It naturally allows for a workflow which could be similar to following:

  • Make changes
  • Test them, Verify compatibility (aka check)
  • Commit
  • Merge
  • Use CI to commit updated .api file

Therefore it poses a slight deviation from "Regular Workflow", but for a good reason. If the developer works on a gargantuan project, they should not be asked to study the project configuration as they are only responsible for making changes and not breaking compatibility. This is in turn a responsibility of the plugin to verify. In other words their time is better spent studying the current domain, instead of the project build harness.

Regular Workflow therefore (speculation) proposes isolated instances of different kinds of build/verification instrumentation for each team or module, as the team would have to know instrumentation tasks to essentially not break compatibility if not done automatically. This is what I'd love to avoid.


I submitted a PR #186 which resolves this problem and allows for incrementally compatible checks only.

Reviewers are welcome to submit notes, questions or improvements for the given PR.

Obviously there's a plethora of options that could be introduced to the gradle extension to expand this feature to the moon. However to keep the scope for this new feature small, I opted for a simple flag which swaps the internal implementation of the apiCheck task.

@fzhinkin
Copy link
Collaborator

I see how useful such a mode could be. However, there are a few concerns worth mentioning.

With a workflow where additions to the .api file don't fail the validation and these changes have to be committed before a release, there are at least two scenarios when things could go wrong:

  • if apiDump + commit is fully automated in CI, some unintended ABI updates may leak into a release. Of course, it's all about the review culture in a project and how meticulous reviewers are, but there will be no safety net guarding against such mistakes anymore.
  • if, for some reason, .api was not updated before a release, BCV will no longer be capable of warning if some recently added declarations will be later removed (i.e., someone added a public class C, it was released without .api being updated, someone later removed class C; since there was no such a class in a dump, BCV won't catch the removal).

This means that developers who are willing to use incremental validation have to opt for it explicitly and should be informed about potential risks.

As a side note:

Therefore it poses a slight deviation from "Regular Workflow", but for a good reason. If the developer works on a gargantuan project, they should not be asked to study the project configuration as they are only responsible for making changes and not breaking compatibility. This is in turn a responsibility of the plugin to verify. In other words their time is better spent studying the current domain, instead of the project build harness.

It's no longer the plugin's responsibility as soon as validation requires some multistep workflow stretched in time and involving multiple cooperating agents to make it work correctly.

While it might seem like a simplification, incremental validation will probably make the overall release process more complex.

@diareuse
Copy link
Author

I agree with multiple, nearly all, points you've made, but let me elaborate anyway.


…some unintended ABI updates may leak into a release.

I agree and that's why I don't envision this being the default anytime soon. I feel like enabling this mode would need thorough consideration and understanding of the potential implications by the implementer. I'm not sure how to warn users sufficiently and if you have any ideas, I'm willing to include them into my PR.

That said I feel like the potential user is a sane person who knows what they are doing, therefore considering the implications based on the mode they have explicitly enabled is out of the question. In other words: I don't believe the user is stupid.

This means that developers who are willing to use incremental validation have to opt for it explicitly and should be informed about potential risks.

I'd cave into giving them suggestion on a good workflow and to advise that branch protections with sufficient checks and automation are absolute must when using this particular mode. But then again I want to believe most of them use this already.

I'm considering this a good workflow and would love to know your opinion about it:

  • PR is opened with new changes
  • API is checked for compatibility, PASS
  • Reviewer requests changes
  • API is changed, pushed
  • API is checked for compatibility, PASS
  • PR is approved
  • API is dumped, committed and merged

It's no longer the plugin's responsibility…

I feel like we have a misunderstanding regarding the responsibilities here. From my point of view, in the current state the plugin is that it's responsible for failing on every single change, therefore it does not verify the compatibility, it verifies the identity. The compatibility responsibility is deferred to the developer which might or might not have the understanding of this particular plugin and how to make the correct decision about the API being compatible.

My intention is to shift this responsibility to the party which understands these changes and make a correct decision for them. We can discuss how to improve this decision-making process, but I would prefer to do this after user feedback due to obvious reasons.

…incremental validation will probably make the overall release process more complex.

I agree, but not in terms of the developer workflow, but the devops team. I'd love to trade in 1 person's afternoon for the team of 30 saving 15 minutes on every PR. Would you not?


In any case, these are great points and thanks for voicing them 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants