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

Setup release process #179

Merged
merged 4 commits into from
Aug 14, 2018
Merged

Setup release process #179

merged 4 commits into from
Aug 14, 2018

Conversation

sigiesec
Copy link
Collaborator

@sigiesec sigiesec commented Aug 14, 2018

Fixes #74

@sigiesec sigiesec requested a review from a team August 14, 2018 08:29
@sigiesec
Copy link
Collaborator Author

@jcoeltjen Please give this a thorough look. It seems to work now, but maybe some things are done in an unusual way or are unnecessary.

@sigiesec sigiesec force-pushed the setup-release-process branch from 10626b3 to 3039457 Compare August 14, 2018 08:37
Copy link
Contributor

@jcoeltjen jcoeltjen left a comment

Choose a reason for hiding this comment

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

Everything seems correct but at the same time somewhat overcomplicated.
I am not that firm with Travis so maybe it has to be that way...

# used by a subsequent build and overwrite the tag
git checkout $TRAVIS_BRANCH

DEVELOPMENT_VERSION=$(python -c 'version = tuple(map(int, ("'${RELEASE_VERSION}'".split(".")))); print("%i.%i.%i" % (version[0], version[1]+1, 0))')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use python here when you you sed in all other places?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My sed knowledge is quite limited. I copied the sed expression from somewhere, but I wrote this myself ;) In addition, I think incrementing a number is not straight-forward with sed.


cd com.btc.serviceidl.plainjava && mvn versions:set -DnewVersion=${BASE_VERSION}${BRANCH_FRAGMENT}${SNAPSHOT_FRAGMENT}

mvn deploy -DskipTests
Copy link
Contributor

Choose a reason for hiding this comment

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

mvn deploy could be moved outside the if/else because it is executed in both branches.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, if release:perform is not used, this could be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will leave it at is it for now. Maybe #181 will change this again.

SNAPSHOT_FRAGMENT="-SNAPSHOT"
BASE_VERSION=`mvn org.apache.maven.plugins:maven-help-plugin:2.1.1:evaluate -Dexpression=project.version | sed -n -e '/^\[.*\]/ !{ /^[0-9]/ { p; q } }' | sed -e 's/-SNAPSHOT//'`

if [ "$TRAVIS_PULL_REQUEST" == "false" ] ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this checked here?
Is the deploy.sh script even executed on pull request builds and if yes, why?

BRANCH_FRAGMENT="-pr${TRAVIS_PULL_REQUEST}"
fi

cd com.btc.serviceidl.plainjava && mvn versions:set -DnewVersion=${BASE_VERSION}${BRANCH_FRAGMENT}${SNAPSHOT_FRAGMENT}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you build the new version from multiple fragments?
Is is not possible to use RELEASE_VERSION or DEVELOPMENT_VERSION for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not changed as part of this PR, it was only moved into the if and the indentation was changed.

RELEASE_VERSION and DEVELOPMENT_VERSION are not set at all in this block.

BRANCH_FRAGMENT=""
cp .travis.settings.xml $HOME/.m2/settings.xml

if [[ "$TRAVIS_BRANCH" =~ ^release/ ]] ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Is TRAVIS_BRANCH set if building a PR?
If not you can skip the check in line 55

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is set for PR builds: "for builds triggered by a pull request this is the name of the branch targeted by the pull request."

@sigiesec
Copy link
Collaborator Author

@jcoeltjen What in particular do you think is overly complicated and smells like it could be simplified?

@sigiesec sigiesec force-pushed the setup-release-process branch from 3039457 to 1fd7d73 Compare August 14, 2018 10:23
@sigiesec sigiesec merged commit a139a7d into master Aug 14, 2018
@sigiesec sigiesec deleted the setup-release-process branch August 14, 2018 11:25
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