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

Adding a key for the pr build # #12

Merged
merged 3 commits into from
Feb 25, 2017
Merged

Conversation

EmilDafinov
Copy link
Contributor

@EmilDafinov EmilDafinov commented Feb 25, 2017

Adding a task key that allows to determine of the build is a pr build and what is its number. Allows PR builds to be handled differently.

… is a pull request. Allows to treat pr builds differently (for example, we might not want to deploy artifacts in a PR build)
@@ -14,7 +18,11 @@ object TravisCiPlugin extends AutoPlugin {
override def trigger = allRequirements

override def globalSettings = Seq(
isTravisBuild := sys.env.get("TRAVIS").isDefined)
isTravisBuild := sys.env.get("TRAVIS").isDefined,
prBuildNumber := Try {
Copy link
Owner

Choose a reason for hiding this comment

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

Looks good. But I want to prefix the keys we introduce (it's a plugin best practice), so WDYT of travisPrNumber?

Copy link
Contributor Author

@EmilDafinov EmilDafinov Feb 25, 2017

Choose a reason for hiding this comment

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

Sounds good to me. Thanks for the quick feedback :)

… is a pull request. Allows to treat pr builds differently (for example, we might not want to deploy artifacts in a PR build)
@dwijnand
Copy link
Owner

@EmilDafinov, looks good.

One more thing (sorry I should've mentioned this on my first feedback): could you document this in the README as well as in a release notes file under notes/1.1.0/*.markdown please? Thanks.

@EmilDafinov
Copy link
Contributor Author

@dwijnand, I added a 1.1.0.markdown file in the notes, next to the existing 1.0.0.markdown. Is that alright, or would you prefer to have them in separate sub-directories?

@dwijnand
Copy link
Owner

That's fine.

I'm using here the same technique that we use in sbt. But in sbt we ended up quickly running into silly merge conflict warnings on release notes. So we use release notes directories for in-progress releases. Here things are more quiet, so we'll most likely not hit the issue.

Thanks. LGTM.

@dwijnand dwijnand merged commit 1f82e1b into dwijnand:master Feb 25, 2017
@EmilDafinov
Copy link
Contributor Author

@dwijnand Quick follow up question, something that didn't occur to me when merging: We documented the new key in the 1.1.0 release notes, I'm assuming that is going to be the version of the plugin it will appear in. However, in the readme incorrectly indicates that it should be available in 1.0.0. Maybe it should not appear in the readme before 1.1.0 is released, what do you think?

@dwijnand
Copy link
Owner

I think I could speak for a long time about this subject, from different angles and perspectives. I've seen many different solutions and I'm not sure which one I prefer.

One way to avoid the whole problem is to just cut a release. Which thanks to using sbt-dynver and my build setup is quite simple. I'll cut 1.1.0.

@dwijnand
Copy link
Owner

Done https://github.com/dwijnand/sbt-travisci/releases/tag/v1.1.0

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