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

SBT 1.0 support #11

Merged
merged 1 commit into from
Aug 16, 2018
Merged

SBT 1.0 support #11

merged 1 commit into from
Aug 16, 2018

Conversation

jeffmay
Copy link
Contributor

@jeffmay jeffmay commented Aug 15, 2018

@rallyhealth/engineers @htmldoug @arkban @rcmurphy

Changes Summary

Refactor

I've separated the MiMa checks from the other checks which vastly simplifies the code. Implementation of semVerCheckMiMa: TaskKey[Unit] is currently just 12 lines. Inputs are 2 version/file pairs, and output is an Exception.

The previous version determination also gets a new maybePrevRelease: TaskKey[Option[ReleaseVersion]]. This handles the git branch state sifting and captures its result.

The other checks (e.g. previous < current) are split into semVerCheckValidVersion: TaskKey[Unit].

semVerPreRelease removed

semVerPreRelease referred to major=0 versions as described in http://semver.org/#spec-item-4, but "pre-release" has an entirely different meaning: http://semver.org/#spec-item-9. Confusing!

I'm removing to simplify. There are enough seams for users to add the capability back in their own build.sbt if desired.

semVerLimit deprecated/ignored

The initial intent behind semVerLimit was to highlight any breaking changes in PR that would require major version bumps. Unfortunately, semVerLimit does not provide sufficient information to other plugins (like the shading plugin https://github.com/AudaxHealthInc/rally-versioning/pull/77) about staged major releases. rallyVersioningSnapshotLowerBound has since superseded it.

semVerLimit is now deprecated and if set, will log a deprecation warning and that the value will be ignored.

Instead, semVerEnforcementLevel: TaskKey[SemVerReleaseType], with possible values of Major/Minor/Patch replaces semVerLimit as the input to MiMa and threshold for analyzing results.

Logging

I have received lots of questions from other engineers about SemVer's logging. It's currently high noise/low signal.

This is a passing test run from an example project:

> test
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Checking ENABLED with LIMIT target semVerLimit=1.999.999
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[info] [SemVer] Check type: comparing most recent release with post-tag changes
[info] [SemVer] Check starting (prev=0.3.0 vs curr=1.999.999) ...
[info] [SemVer] Check aborted: limit=1.999.999 allows major upgrades, no check needed.
[success] Total time: 5 s, completed Sep 5, 2017 11:26:06 PM

New strategy is to include the relevant failure information in the failure exception. On success, we log at debug or below, just like the majority of other sbt tasks.

Rough plan

  • Remove scripted fork
  • Disable failing scripted tests
  • Refactor to change scattered logging side-effects to typed exceptions w/ summaries.

[error] [SemVer] version=2.0.1 FAILED: required diffType=minor NOT achieved
java.lang.IllegalStateException: Proposed RELEASE version=2.0.1 FAILED SemVer rules, failing build
at com.rallyhealth.semver.SemVerHalter.haltBuild(SemVerHalter.scala:86)
at com.rallyhealth.sbt.semver.SemVerHalter.haltBuild(SemVerHalter.scala:86)
...
Copy link
Contributor Author

@jeffmay jeffmay Aug 15, 2018

Choose a reason for hiding this comment

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

I think the SemVerHalter code was removed. This output will be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@htmldoug we should update the internal readme for this as well

@@ -4,35 +4,60 @@ name := "sbt-git-versioning"
organizationName := "Rally Health"
organization := "com.rallyhealth.sbt"

enablePlugins(SemVerPlugin)
// enable after SemVerPlugin supports sbt-plugins
//enablePlugins(SemVerPlugin)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was failing because it couldn't find the 1.0.0 release. I'm guessing that I need to release 1.1.0 with these new settings, then I can try to turn this on.

Copy link

Choose a reason for hiding this comment

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

Perhaps prefix that comment with TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a fast-follow to upgrade this plugin to use sbt 1.0 itself. I'll add this back for that PR (or before, since I might also fast-follow with code coverage).

build.sbt Outdated
// val publishType = if (version.value.contains("-SNAPSHOT")) "snapshot" else "release"
// val (name, url) = (s"ivy-$artifactType-$publishType-local", baseUrl + s"ivy-$artifactType-$publishType-local")
// Some(Resolver.url(name, new URL(url))(Resolver.ivyStylePatterns))
//}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

BLKR: Remove

@arkban
Copy link

arkban commented Aug 15, 2018

semVerLimit is now deprecated and if set, will log a deprecation warning and that the value will be ignored.

I think it's time to just delete it.

.travis.yml Outdated
jdk:
- openjdk8
script:
- sbt +test
- sbt +test scripted

Choose a reason for hiding this comment

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

+scripted to test both sbt versions.

Copy link

@htmldoug htmldoug left a comment

Choose a reason for hiding this comment

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

Looks familiar. LGTM.

Copy link

@arkban arkban left a comment

Choose a reason for hiding this comment

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

I did quick-ish review

.travis.yml Outdated
jdk:
- openjdk8
script:
- sbt +test
- sbt +test scripted
Copy link

Choose a reason for hiding this comment

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

Should that be +scripted?

@@ -1,9 +1,9 @@
**sbt-git-versioning** is an sbt plugin designed to make maintaining a simple, consistent, and accurate
Copy link

Choose a reason for hiding this comment

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

Does this include the various updates to rally-versioning's README? I think I updated that recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I copied it over and refactored any mismatching names or rally-specific comments

resolvers += Resolver.url(
"Rally Plugin Releases",
url("https://dl.bintray.com/rallyhealth/sbt-plugins"))(Resolver.ivyStylePatterns)
resolvers += Resolver.bintrayIvyRepo("rallyhealth", "sbt-plugins")
Copy link

Choose a reason for hiding this comment

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

Do we actually publish to bintray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -26,20 +24,16 @@ In either case, you should now be able to add the plugin dependency to `project/
```scala
addSbtPlugin("com.rallyhealth.sbt" % "sbt-git-versioning" % "x.y.z")
```
3. Enable the plugin and set `semVerLimit` in `build.sbt` (see
Copy link

Choose a reason for hiding this comment

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

Please add stuff about the rallyVersioningSnapshotLowerBound or whatever the equivalent is since I imagine the common path will be adding this to existing libraries and they will need that

...
[error] (*:compile) java.lang.IllegalStateException: Proposed RELEASE version=2.0.1 FAILED SemVer rules, failing build
[error] Total time: 2 s, completed Nov 15, 2016 3:27:36 PM
```

### semVerLimit

Before using the check (manually or automatically), you *must* set the `semVerLimit` key in your `build.sbt`.
This is a version (e.g. "1.2.3") that tells SemVerPlugin when to halt the build. It prevents you from making any
`semVerLimit` is a version (e.g. "1.2.3") that tells SemVerPlugin when to halt the build. It prevents you from making any
Copy link

Choose a reason for hiding this comment

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

We should probably figure out a standard naming convention, some of our configs have the rally prefix and some don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two prefixes are semVer and gitVersioning

@@ -2,5 +2,5 @@ resolvers += Resolver.url(
"Rally Plugin Releases",
url("https://dl.bintray.com/rallyhealth/sbt-plugins"))(Resolver.ivyStylePatterns)

addSbtPlugin("com.rallyhealth.sbt" % "sbt-git-versioning" % "0.1.0")
addSbtPlugin("me.lessis" % "bintray-sbt" % "0.3.0")
addSbtPlugin("com.rallyhealth.sbt" % "sbt-git-versioning" % "1.0.0")
Copy link

Choose a reason for hiding this comment

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

This plugin depends on itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. It's bootstrapped.

* the root is not a real project. That path comes from [[MimaKeys.mimaCurrentClassfiles]]. I do not know why it
* contains a non-existent path if the failure is so harsh.
* Unfortunately I could not re-create this with a unit or scripted test either. I only replicated it in the wild
* with lib-carestats. If I ran "sbt compile" it would just halt SBT trying to compile the root project, specifically
Copy link

Choose a reason for hiding this comment

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

Remove "lib-carestats" references

// This Plugin does not directly support problem filters that are in MiMa, see "problemFilter" field in
// https://github.com/typesafehub/migration-manager/blob/master/reporter/src/main/scala/com/typesafe/tools/mima/cli/Main.scala
// Currently all the Problem analyzers are useful so at the time of writing we had no need for filtering
}

import autoImport._

lazy val semVerVersion: SettingKey[SemanticVersion] = settingKey[SemanticVersion](
Copy link

Choose a reason for hiding this comment

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

Should this one also be hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I mostly just copied this from our internal plugin development.

@@ -6,7 +6,7 @@
$ exec git init
$ exec git add .
$ exec git commit -am 'Initial commit.'
$ exec git tag v0.0.1
$ exec git tag v1.0.1
Copy link

Choose a reason for hiding this comment

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

Do you have another test like this that does tagging at v0? I wouldn't want to lose coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@htmldoug should we add this test back to the plugin? If so, that should go to the internal plugin version.

@@ -10,9 +10,9 @@ Initialized empty Git repository in /private/var/folders/vp/p4d6nc0d7bnctv6j0lgr
[info]  5 artifacts copied, 0 already retrieved (24494kB/49ms)
Copy link

Choose a reason for hiding this comment

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

It would be nice to strip all these stupid color codes...

- cross compile to sbt 1.0 and sbt 0.13
- semVerPreRelease removed
- semVerLimit deprecated/ignored
- Bugfix: gitVersioningSnapshotLowerBound always produces a SNAPSHOT
  anytime it modifies the version field even if HEAD revision is tagged
- Fix travis build to run scripted tests
@jeffmay jeffmay requested a review from rcmurphy August 15, 2018 23:11
@jeffmay jeffmay merged commit 118afbb into master Aug 16, 2018
@jeffmay jeffmay deleted the sbt-1-support branch August 16, 2018 01:21
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.

3 participants