Add CD build pipeline #1

sherifnada · 2019-01-08T10:33:59Z

Context

CD pipeline should be ready to go now. To release a new version of this lib, we'll probably want to use the maven release plugin which moves the current version off of -SNAPSHOT. It'll temporarily change e.g 1.0-SNAPSHOT to 1.0, perform a deploy, then bump to 1.1-SNAPSHOT to resume development. It also has a bunch of other handy features that make life easier when deploying.

Structure of changes

.travis.yml: in order for the build to succeed, two things need to happen successfully:

build and test the module
sign artifacts and deploy them to Sonatype (which will mirror to Maven Central)

pom.xml: A few small details that the build needs. Perhaps most interesting is the sign profile.

cd/mvnsettings.xml: Specifies the environment variables that will be injected at build time for use in signing and publishing artifacts.

*.java: just a bunch of changes I had to make to get the build working. Ignore the java files since they'll be revamped in future PRs this week.

Misc

I'd love any comments on readability/accessibility of the non java code.

Potential Improvements

I think it would also be good to add some notes in the README or elsewhere specifying the dev workflow when editing this repo (i.e: how do you make sure everything short of deploying to maven central works? How do you release etc..).

….asc.enc containing encrypted GPG keys

… phase

This way, if publishing fails, then so will the build.

bpodgursky · 2019-01-08T18:16:14Z

pom.xml

  <build>
+    <!-- Be explicit about where maven should look for source and test files -->
+    <sourceDirectory>src/main/java</sourceDirectory>


isn't this the default?

It is. I thought it was confusing that the Maven default is for tests to have a different structure (test/java) than source code (src/main/java), so I wanted to make these two lines explicit just to be clear.

Though this being said, my hunch is that while it may be nice in the short term, over time this kind of cruft (explicit specification instead of convention) could build up. This makes me think there's no reason to even use the non-default directories since it just means I have to add more to my POM without much of a gain (if it's confusing that src and tst have different dirs, well, blame it on Maven :D ).

Do you think it's better to call these out like this or just use convention? You've definitely had more experience maintaining POMs than I have.

I would strongly prefer to use maven defaults unless we have functional reasons for changing it.

ChristianHansen

Left some comments, mostly nit-picks.
Thanks for getting this working and including comments in the pom.

ChristianHansen · 2019-01-08T19:23:34Z

cd/deploy.sh

+  openssl aes-256-cbc -K $encrypted_9718d3d48d9b_key -iv $encrypted_9718d3d48d9b_iv -in $TRAVIS_BUILD_DIR/cd/codesigning.asc.enc -out $TRAVIS_BUILD_DIR/cd/codesigning.asc -d || exit 1;
+
+  # import these certs into GPG
+  gpg --fast-import $TRAVIS_BUILD_DIR/cd/codesigning.asc || exit 1;


You could do set -e at the beginning of this script so it exits if any commands return a non-zero exit code. That way, you don't need the || exit 1.

thanks for the tip!

ChristianHansen · 2019-01-08T19:24:46Z

cd/deploy.sh

+  gpg --fast-import $TRAVIS_BUILD_DIR/cd/codesigning.asc || exit 1;
+
+  # Remove the unencrypted cert file
+  rm cd/codesigning.asc || exit 1;


Do we want to ensure this is always deleted, no matter what?
You could do trap 'rm cd/codesigning.acs' EXIT right after the openssl command to ensure this file is always deleted on exit.

ChristianHansen · 2019-01-08T19:29:35Z

cd/deploy.sh

+  # Note that if any of these commands fail, we exit so we get quick feedback from the build.
+
+  # First, unencrypt the certs we'll use to sign artifacts
+  openssl aes-256-cbc -K $encrypted_9718d3d48d9b_key -iv $encrypted_9718d3d48d9b_iv -in $TRAVIS_BUILD_DIR/cd/codesigning.asc.enc -out $TRAVIS_BUILD_DIR/cd/codesigning.asc -d || exit 1;


I believe it's generally a best practice to put double-quotes around interpolated vars in Bash to prevent word-splitting or globbing. Also, I'd recommend using curly braces e.g. ${TRAVIS_BUILD_DIR} in general, since it makes it very explicit what the interpolated var is an prevents sneaky bugs from typos like $TRAVIS_BUILD_DIRcd evaluating to "".

I use shellcheck to help catch stuff like this.

Awesome, thanks for pointing out shellcheck. These are all good points.

ChristianHansen · 2019-01-08T19:33:46Z

pom.xml

+              <execution>
+                <phase>package</phase>
+                <goals>
+                  <goal>shade</goal>


Are we just uber-jarring here, or is there actual shading needed? Can we have comment explaining this?

ChristianHansen · 2019-01-08T19:35:57Z

test/main/java/com/liveramp/hyperminhash/TestBetaMinHash.java

 import org.junit.Test;

+import java.nio.charset.StandardCharsets;


Let's talk offline, but we should probably sync on our code-formatting settings so we don't have a bunch of formatting only-diffs

ChristianHansen · 2019-01-08T19:36:50Z

test/main/java/com/liveramp/hyperminhash/TestBetaMinHash.java

      long result = BetaMinHash.intersection(sk1, sk2);

      double pctError = 100 * getError(result, expected);
      double expectedPctError = 5.0;
-      assertTrue(String.format("Expected error ratio to be at most %s but found %f", expectedPctError, pctError),
+      assertTrue(
+          String.format("Expected error ratio to be at most %s but found %f", expectedPctError,


I assume this was just the IntelliJ formatter gone rogue, but it would be nice to stick to one-arg-per-line if we're splitting over multiple lines.

ChristianHansen · 2019-01-08T19:37:00Z

test/main/java/com/liveramp/hyperminhash/TestBetaMinHash.java

@@ -144,7 +143,8 @@ public void testManyWayIntersection() {
      long actualIntersection = BetaMinHash.intersection(sk1, sk2, sk3);
      double pctError = 100 * getError(actualIntersection, expectedIntersection);
      assertTrue(
-          String.format("Expected pctError to be <2, found %f. Expected: %d, Actual: %d", pctError, expectedIntersection, actualIntersection),
+          String.format("Expected pctError to be <2, found %f. Expected: %d, Actual: %d", pctError,


same comment as above

ChristianHansen · 2019-01-08T19:38:25Z

.travis.yml

+script:
+  # First install any deps and runs tests
+  - mvn clean install test -P-build-src-and-docs -B -V


could we package instead of install? (not sure)

changed to package since there is no need to install into the .m2 cache

ChristianHansen

Given that the Java formatting changes are addressed in #2, LGTM

ChristianHansen · 2019-01-11T17:30:54Z

cd/deploy.sh

+#! /usr/bin/env bash
+
+# TODO comment this back in
+#if [ "$TRAVIS_BRANCH" = 'master' ] && [ "$TRAVIS_PULL_REQUEST" == 'false' ]; then


comment this back in?

…rep-release

Shrif Nada added 28 commits January 7, 2019 14:13

prepare POM for publishing to maven central

2313ce1

add sonatype snapshots

9391b8d

add maven settings containing env variable key names, and codesigning…

9bbe315

….asc.enc containing encrypted GPG keys

add sonatype staging plugin

9b186d3

fix typo

6e8511f

remove need for apache commons-codec and hadoop writable

22e401c

dont inhert from liveramp commons pom

8f7f76b

add travis deploy instructions

3bb5dc1

fix typo

740f0da

add necessary plugins and profiles for release

de375a1

add WIP deploy script for importing secrets and deploying artifacts

30a81ed

make cd/deploy.sh executable

b5471c9

use absolute paths in deploy script

f6b003e

bump test error bound to 2.5%

6892f6d

fix linting errors

85e1ad2

disable failing over lint

7c4b24b

skip tests when deploying since we already run tests during the build…

37dec58

… phase

fix linting issues

614e53a

fix lint

448da29

add escaped secret

0053466

remove empty <p> tags

2b1e4da

remove doclint disable profile and encrypted travis key

50344c3

verbose maven output

efc4a46

remove whitespace

2dc564d

fix snapshot repository IDs

726aaa6

Make publishing part of main build script.

6a7daa9

This way, if publishing fails, then so will the build.

don't print debug output when deploying

31f8af0

add comments for readability

2041204

bpodgursky reviewed Jan 8, 2019

View reviewed changes

ChristianHansen reviewed Jan 8, 2019

View reviewed changes

Shrif Nada added 12 commits January 8, 2019 12:57

move test dir to default maven dir

1dd6844

package instead of install during build

e4157db

use env gpg cert instead of encrypted cert file

22acde0

remove Writable reference from README

3abe674

formatting

49b4a12

formatting

d68ab58

move tests to correct dir

00c9ffc

formatting

d6eb021

add comments explaining use of shading plugin

e31ca89

add echo statements for debugging

3d12c2c

delete encrypted cert

67704d0

explicitly specify mvn settings dir

8c3160b

ChristianHansen approved these changes Jan 11, 2019

View reviewed changes

Shrif Nada added 4 commits January 11, 2019 15:44

Merge branch 'master' of github.com:LiveRamp/HyperMinHash-java into p…

6e6c99d

…rep-release

fix conflict

34ea4e9

fix tests

349a20b

only upload to sonatype if we're on master

61ae497

sherifnada merged commit f668b03 into master Jan 12, 2019

sherifnada deleted the prep-release branch February 21, 2019 23:19

joshk0 mentioned this pull request May 29, 2019

get rid of repository.liveramp.com profile #12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CD build pipeline #1

Add CD build pipeline #1

sherifnada commented Jan 8, 2019 •

edited

Loading

bpodgursky Jan 8, 2019

sherifnada Jan 8, 2019

bpodgursky Jan 8, 2019

sherifnada Jan 8, 2019

ChristianHansen left a comment

ChristianHansen Jan 8, 2019

sherifnada Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

sherifnada Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

ChristianHansen Jan 8, 2019

sherifnada Jan 8, 2019

ChristianHansen left a comment

ChristianHansen Jan 11, 2019

		import org.junit.Test;

		import java.nio.charset.StandardCharsets;

Add CD build pipeline #1

Add CD build pipeline #1

Conversation

sherifnada commented Jan 8, 2019 • edited Loading

Context

Structure of changes

Misc

Potential Improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianHansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianHansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherifnada commented Jan 8, 2019 •

edited

Loading