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

Allow multiple exclude files per version to be loaded from a directory #239

Merged
merged 2 commits into from Oct 4, 2018

Conversation

jrudolph
Copy link
Contributor

So, instead of creating a single

mima-filters/10.1.5.backwards.excludes

file to which everyone appends, you can now create a directory with the same name containing multiple exclude files per change like that:

mima-filters/10.1.5.backwards.excludes/pr1234.excludes
mima-filters/10.1.5.backwards.excludes/pr5678.excludes

etc.

This will prevent the common merge conflicts when everyone tries to append to the same file when in fact the ordering of entries is not interesting.

I'm thinking about also adding another task that would collect all those files
from the directory into a single file once the "version is closed", i.e. a new one was created to prevent a huge amount of filter files accruing over time.

So, instead of creating a single

mima-filters/10.1.5.backwards.excludes

file to which everyone appends, you can now create a directory with the same name
containing multiple exclude files per change like that:

mima-filters/10.1.5.backwards.excludes/pr1234.excludes
mima-filters/10.1.5.backwards.excludes/pr5678.excludes

etc.

This will prevent the common merge conflicts when everyone tries to append
to the same file when in fact the ordering of entries is not interesting.
@jrudolph
Copy link
Contributor Author

I'm thinking about also adding another task that would collect all those files
from the directory into a single file once the "version is closed", i.e. a new one was created to prevent a huge amount of filter files accruing over time.

WIP on that is here: https://github.com/jrudolph/migration-manager/tree/jr/add-collection-task

Copy link
Collaborator

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Great idea!

@2m
Copy link
Contributor

2m commented Sep 27, 2018

Great idea! I wonder if the directory name could be improved as there is some duplication right now. Something like:

mima-filters/10.1.5/pr1234.backwards.excludes
mima-filters/10.1.5/pr5678.backwards.excludes

@2m
Copy link
Contributor

2m commented Sep 27, 2018

... to prevent a huge amount of filter files accruing over time.

I do not think that is a big problem for git or for navigating through these files. They will even be named by the PR that introduced the incompatibility which greatly increases manageability.

Also we can delete filter files for a version, when it is not being used to check compatibility anymore.

@jrudolph
Copy link
Contributor Author

jrudolph commented Oct 2, 2018

I wonder if the directory name could be improved as there is some duplication right now. Something like:

mima-filters/10.1.5/pr1234.backwards.excludes
mima-filters/10.1.5/pr5678.backwards.excludes

Seems to increase duplication for me ;)

I do not think that is a big problem for git or for navigating through these files. They will even be named by the PR that introduced the incompatibility which greatly increases manageability.

Also we can delete filter files for a version, when it is not being used to check compatibility anymore.

That's right, checking against old versions isn't too useful. It would need a pattern of changing (or deleting / recreating) the same API multiple times before it would actually catch anything. So, I guess it makes sense to remove old entries regularly anyway.

@2m
Copy link
Contributor

2m commented Oct 2, 2018

Seems to increase duplication for me ;)

It has twice as little excludes! :)

@jrudolph
Copy link
Contributor Author

jrudolph commented Oct 2, 2018

It has twice as little excludes! :)

Ah, I see, but it's double backwards :)

@2m
Copy link
Contributor

2m commented Oct 2, 2018

Yea, but it is the same number of backwards as it was in the original proposal.

@jrudolph
Copy link
Contributor Author

jrudolph commented Oct 2, 2018

How about not allowing any other files in those directories, then it could be

mima-filters
 |- 10.1.5.backwards.excludes
    |- pr1224
    |- pr5678

@jrudolph
Copy link
Contributor Author

jrudolph commented Oct 4, 2018

Now it takes every file from the dir removing all the duplication in file names.

Copy link
Contributor

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM. Awesome!

@2m 2m merged commit 2573cee into lightbend-labs:master Oct 4, 2018
@2m 2m added this to the 0.3.1 milestone Oct 4, 2018
dwijnand added a commit that referenced this pull request Oct 11, 2018
…0.3.0', 'scala-steward/update/sbt-compat-1.2.6', 'scala-steward/update/config-1.3.3', 'scala-steward/update/config-1.0.2' and 'scala-steward/update/sbt-bintray-0.5.4'

* scala-steward/update/sbt-mima-plugin-0.3.0:
  Update sbt-mima-plugin to 0.3.0
  Allow multiple exclude files per version to be loaded from a directory (#239)
  Only handle NonFatal exceptions
  Don't swallow the cause
  Produce a better error message when files are found that cannot be processed
  Run it:test on Scala 2.13.0-M4 in CI

* scala-steward/update/sbt-compat-1.2.6:
  Update sbt-compat to 1.2.6
  Allow multiple exclude files per version to be loaded from a directory (#239)
  Only handle NonFatal exceptions
  Don't swallow the cause
  Produce a better error message when files are found that cannot be processed
  Run it:test on Scala 2.13.0-M4 in CI

* scala-steward/update/config-1.3.3:
  Update config to 1.3.3
  Allow multiple exclude files per version to be loaded from a directory (#239)
  Only handle NonFatal exceptions
  Don't swallow the cause
  Produce a better error message when files are found that cannot be processed
  Run it:test on Scala 2.13.0-M4 in CI

* scala-steward/update/config-1.0.2:
  Update config to 1.0.2
  Allow multiple exclude files per version to be loaded from a directory (#239)
  Only handle NonFatal exceptions
  Don't swallow the cause
  Produce a better error message when files are found that cannot be processed
  Run it:test on Scala 2.13.0-M4 in CI

* scala-steward/update/sbt-bintray-0.5.4:
  Update sbt-bintray to 0.5.4
  Allow multiple exclude files per version to be loaded from a directory (#239)
  Only handle NonFatal exceptions
  Don't swallow the cause
  Produce a better error message when files are found that cannot be processed
  Run it:test on Scala 2.13.0-M4 in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants