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

Support establishing the actual level of compatibility #179

Conversation

rtyley
Copy link
Contributor

@rtyley rtyley commented Sep 21, 2023

I'd like to enable a workflow like this for developers working on libraries (for us at the Guardian, repos like content-api-scala-client, etag-caching, etc):

  1. Keep devs aware of the compatibility of the PR they're working on : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that "This PR breaks binary/source compatibility" - the information here would come from the sbt-version-policy plugin, but wouldn't require setting a versionPolicyIntention - it just establishes what the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).
  2. Required version increment automatically determined at release : When merged, the developer can trigger an automated maven-publication workflow on the main branch. As part of that process, the level of compatibility with the previous release would again be established by sbt-version-policy, determining a resulting version number bump (major, minor, or patch), with the workflow determining what the resulting release version number should be, rather than the developer.

Computing Compatibility without setting versionPolicyIntention

The main documented mode of operation with sbt-version-policy at the moment requires a target sbtversionpolicy.Compatibility to be selected by the developer with versionPolicyIntention, and then sbt-version-policy informs the developer whether or not they are hitting that target.

To enable the workflow described in this PR, we need a way to compute that sbtversionpolicy.Compatibility without actually setting a versionPolicyIntention, as mentioned in #109

So far as I can see Compatibility depends on two things; MIMA-detected changes, and dependencies:

versionPolicyCheck := Def.ifS((versionPolicyCheck / skip).toTask)(Def.task {
()
})(Def.task {
val ignored1 = versionPolicyMimaCheck.value
val ignored2 = versionPolicyReportDependencyIssues.value
}).value,

It looks like both of those need to be altered in order to support calculating Compatibility without versionPolicyIntention.

@julienrf
Copy link
Collaborator

Hello, thank you for opening the pull request. It seems your use case is the same as the one in #121, right? I guess that is a legitimate use case. Did you have a look at #121? How does your approach compare to it?

@rtyley rtyley force-pushed the support-merely-establishing-the-level-of-compatability branch from 51f9e7c to f128341 Compare September 21, 2023 14:10
@rtyley
Copy link
Contributor Author

rtyley commented Sep 21, 2023

It seems your use case is the same as the one in #121, right? I guess that is a legitimate use case. Did you have a look at #121? How does your approach compare to it?

Hi @julienrf, thanks for looking in on this - unwisely, I hadn't looked at pre-existing issues/PRs, so really great to know what's already there! I've fleshed out the description of this PR a bit so you can see what I'm trying to achieve.

At the moment I'd just be happy to achieve the first of the two goals - keeping devs aware of the compatibility of their PR changes - and I think this must require implementing #109 (as far as #121 goes, that seems to be focused on getting more detailed/custom reporting, addressing issue #119 - rather than calculating the overall compatibility as in #109).

My understanding is that the calculated value of Compatibility would depend on both MIMA-detected changes, and dependency changes - at the moment, as of f128341, I've just worked on the MIMA-detected changes, introducing a versionAssessMimaCompatibility task key, but I guess I will also need to introduce a versionAssessDependencyCompatibility task, and finally a versionAssessOverallCompatibility task.

It would be great to know if this sounds like a reasonable change to sbt-version-policy, and if my assumptions are correct! If you have time and feel it would be helpful, I'd be happy to jump on a video call to talk it through.

Copy link
Collaborator

@julienrf julienrf left a comment

Choose a reason for hiding this comment

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

Thank you for the commits and the detailed explanation.

I think it is a good idea to add the ability to assess the level of compatibility. I believe that producing compatibility reports is very close to the need of assessing the level of compatibility, so I suggest we keep both goals in minds when designing the solution.

Would it be possible to expose just one new task that computes the compatibility level and returns the incompatibilities found along the way? We could try to be consistent with mima and call it versionFindIncompatibilities. That task would return a data structure containing both the strongest Compatibility value that holds and the possible incompatibilities found along the way.

Other than that, I think your implementation does the right thing, but the logic is pretty complicated to follow since it is spread over the files Compatibility.scala and SbtVersionPolicySettings.scala. Additionally, your changes in Compatibility.scala introduce a (cyclic) dependency on the plugin, which is not desirable. Would it be possible to implement most of the logic outside the sbt plugin and then implement the sbt task by calling that piece of logic with the right parameters?

@rtyley rtyley force-pushed the support-merely-establishing-the-level-of-compatability branch from f128341 to e61b0b8 Compare October 24, 2023 15:10
@julienrf
Copy link
Collaborator

Closing as superseded by #184

@julienrf julienrf closed this Nov 29, 2023
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
rtyley added a commit to guardian/etag-caching that referenced this pull request Nov 29, 2023
This is a stub script that doesn't actually output true compatibility
information, it's just a demonstration showing what kind of behaviour
would be desirable.

It's aiming to show the part 1 of the workflow described in
scalacenter/sbt-version-policy#179:

> **Keep devs aware of the compatibility of the PR they're working on** : When a PR is opened on a library repository, an automated workflow comments-on or labels the PR, making them aware that _"This PR breaks binary/source compatibility"_ - the information here would come from the `sbt-version-policy` plugin, but wouldn't require setting a `versionPolicyIntention` - it just establishes _what_ the level of compatibility the PR entails, so that the developer is aware. The assumption here is that whatever the developer is doing, they need to do it, and should just know the compatibility cost the PR is going to have (and possibly, though not necessarily, once informed they may be able to modify the PR to improve the compatibility).

The script is written to add-and-remove labels as appropriate, if
the required compatibility label (eg 'Breaks Binary Compatibility') wasn't already
present, we assume this is new information and leave a detailed comment.
In general, to reduce noise, we wouldn't necessarily want to add a new
comment on every push, better to just tell the user how they can get this
information by running locally...
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.

2 participants