227 fix version header by heshamMassoud · Pull Request #228 · commercetools/commercetools-sync-java

heshamMassoud · 2018-01-08T14:37:01Z

Summary

Addresses mainly #227.

Instead of depending on JAR manifest for fetching library version and name. Which could be removed by the users of the library, especially in the case when packing it as a dependency in a fat jar, it's quite common that the lib's manifest is replaced with the original applications' manifest.

Therefore, the SolutionInfo version is now injected by a bash script (which potentially takes the version from the tag name, if the build was caused by a tag)

More details

The template string #{LIB_VERSION} in the SyncSolutionInfo.java is replaced in the bash script here.

This sed replace command makes a backup of the original file after replacement, in case a dev runs the script locally. But it's also ignored from committing on the repo.

Relevant Issues

#227 #193

Todo

Add Release Notes entry.

Update (FYI: @butenkor, @andrii-kovalenko-ct):

Now it is written as a gradle script: version-setter.gradle
Which is only called manually by travis before compiling (https://github.com/commercetools/commercetools-sync-java/pull/228/files#diff-354f30a63fb0907d4ad57269548329e3)

… build is triggered by a tag.

… string which is potentially injected by bash script.

codecov-io · 2018-01-08T15:09:19Z

Codecov Report

Merging #228 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master    #228      +/-   ##
===========================================
+ Coverage     93.12%   93.2%   +0.07%     
+ Complexity      805     802       -3     
===========================================
  Files            68      68              
  Lines          2080    2075       -5     
  Branches        120     118       -2     
===========================================
- Hits           1937    1934       -3     
  Misses          122     122              
+ Partials         21      19       -2

Impacted Files	Coverage Δ	Complexity Δ
...ercetools/sync/commons/utils/SyncSolutionInfo.java	`100% <100%> (+22.22%)`	`1 <0> (-3)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d95b3d4...b54b513. Read the comment docs.

butenkor

Besides one typo looks good 👍

butenkor · 2018-01-08T15:28:34Z

+
+**Bug Fixes** (1)
+- **Commons** - Fixed library version in User-Agent headers of JVM SDK clients using the library. Now it's fetched
+ from the JAR manifest but injected from set-solutioninfo-version.sh [#227](https://github.com/commercetools/commercetools-sync-java/issues/227)


-> now it is not fetched

andrii-kovalenko-ct

Or i don't understand the real issue, of the solution is an overhead in the library....
But even if you decided to replace tokens, why do you implement custom shell scripts, instead of using default gradle plugins for replacements?
https://www.google.de/search?q=gradle+replace+plugin

https://stackoverflow.com/a/30038751/907576 (the answer from known Opal guy :) )

heshamMassoud · 2018-01-08T17:06:01Z

@andrii-kovalenko-ct Is there another advantage to having it as a gradle script as opposed to bash script, other than for consistently having all scripts in one place/language?

If so, then we should also better change this gradle as well (https://github.com/commercetools/commercetools-sync-java/blob/master/travis-publish.sh) right?

…fail build to catch failty releases.

butenkor · 2018-01-09T08:04:18Z

 jdk:
 - openjdk8
 install: true # skips travis' default installation step which executes gradle assemble.
+before_script: ./gradlew injectVersion


Why inject and not just setReleaseVersion?

168c067, ac9470f

butenkor · 2018-01-09T08:04:48Z

 apply from: "$rootDir/gradle-scripts/javadocs-publish.gradle"
-apply from: "$rootDir/gradle-scripts/execution-order.gradle"
+apply from: "$rootDir/gradle-scripts/execution-order.gradle"
+apply from: "$rootDir/gradle-scripts/version-setter.gradle"


Also here: set-release-version.gradle

butenkor · 2018-01-09T08:07:05Z

+        if (!versionFileContents.contains(versionPlaceholder)) {
+            throw new InvalidUserCodeException("$versionFile does not contain the placeholder: $versionPlaceholder. " +
+                    "Please make sure the file contains the placeholder, in order for the version to be injected " +
+                    "correctly.")


andrii-kovalenko-ct · 2018-01-09T09:19:12Z

@heshamMassoud

Is there another advantage to having it as a gradle script as opposed to bash script, other than for consistently having all scripts in one place/language?

Isn't it enough as an advantage? Then also:

having it is in gradle makes it platform independent, when bash scripts are platform dependent
bash language is less readable than groovy
less code because standard plugins incapsulate some logic
common plugins are pretty known by other developers, when custom script require time to investigate/understand how they work (that was the first thing i noticed: i need to read bash script, instead of some standard configuration, like in ant replace plugin)
common plugins are used/tested by hundreds of other users - bugs/vulnerabilities are discovered better/faster

If so, then we should also better change this gradle as well (https://github.com/commercetools/commercetools-sync-java/blob/master/travis-publish.sh) right?

Nice point! I've been dreaming to re-factor this wasp nest since my first days in the company, but never had enough time. Also, as a best approach - we could try to create our own travis publish gradle plugin. A good topic for the next hackaton :)

andrii-kovalenko-ct · 2018-01-09T09:25:04Z

 - openjdk8
 install: true # skips travis' default installation step which executes gradle assemble.
-before_script: ./set-solutioninfo-version.sh
+before_script: ./gradlew injectVersion


why not make it as a part of build tasks graph (e.g. using dependsOn, must/shouldRunAfter, finalizedBy and so on).

Because I only want the release version to be replaced only on travis, but not locally on every dev's machine when they run the build locally.

then i'd recommend to create a custom gradle task (for example, ciBuild) and run it from travis (here instead of clean build, avoiding before_script block

This approach makes build script independent from CI tool (e.g. you don't depend on Travis's native before_script)

It is already a custom gradle task. I made it inline now: 62bddb4, but also applies to the after_success then, no?

but also applies to the after_success

good point!

(but don't over-engineer: estimate is it easy to re-factor now all those scripts, or maybe make a dedicated task for this)

andrii-kovalenko-ct · 2018-01-09T09:31:04Z

+                    "correctly.")
+        }
+
+        if (isPr.equals("false") && tagName) {


use groove goodness :) :

isPr == "false"

http://mrhaki.blogspot.de/2009/09/groovy-goodness-check-for-object.html

168c067, I don't think I need the PR check at all actually.

andrii-kovalenko-ct · 2018-01-09T09:44:55Z

+
+        if (libVersion) {
+            println "Injecting the version: '$libVersion' in $versionFile"
+            ant.replace(file: versionFile, token: versionPlaceholder, value: libVersion)


why not default gradle ReplaceTokens ?

because, as I understand, it is used within the filter function when files are being copied. Whereas in this case I just want to replace in file, rather then replace while copying.

heshamMassoud · 2018-01-09T10:09:24Z

Isn't it enough as an advantage?

@andrii-kovalenko-ct It is definitely enough :) but I just wanted to know the advantages. Thanks for explicitly stating them. Also great idea with creating our own gradle script!

I created an issue about it: #229

andrii-kovalenko-ct · 2018-01-09T10:33:11Z

by default tasks order in gradle build is undefined, so it might be that build is executed before setReleaseVersion , even if you specify them in command line in explicit order.

andrii-kovalenko-ct · 2018-01-09T10:34:18Z

http://trickyandroid.com/gradle-tip-3-tasks-ordering/
https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:ordering_tasks

heshamMassoud · 2018-01-09T10:50:52Z

@andrii-kovalenko-ct ah, I didn't know that it also doesn't ensure order when running explicitly from CLI:

FYI, Take a look at this tho: https://discuss.gradle.org/t/ordering-tasks-on-the-command-line-versus-using-mustrunafter/12017

Command line ordering is essentially the same as shouldRunAfter

But still defining explicit ordering is better than relying on CLI input. dc27248 Thanks for pointing it out.

andrii-kovalenko-ct · 2018-01-09T13:10:53Z

hm, i was sure command line order doesn't influence the order at all....

heshamMassoud added 12 commits January 8, 2018 14:55

#227: Remove SyncSolutionInfoTest.

61ca02f

#227: Add bash script to inject version into SyncSolutionInfo.java if…

0e39e0f

… build is triggered by a tag.

#227: Remove fetching lib info from manifest and instead use constant…

c5ed21c

… string which is potentially injected by bash script.

#227: Invoke bash script from travis before gradle build script.

261e649

#227: Fail script on first error.

b5064af

#227: Use correct var in message.

dd0d7c7

#227: Pad message to be more visible.

c053b70

#227: Use printf instead of echo for printing line breaks in string.

36df34a

#227: Remove extra "" string in sed command.

b1e51b8

#227: Backup replaced file and ignore .bak files.

cef5788

#227: Remove extra space before .bak

a38fad8

#227: Add release notes entry.

61c33a8

heshamMassoud requested review from andrii-kovalenko-ct, butenkor and lojzatran January 8, 2018 14:44

butenkor approved these changes Jan 8, 2018

View reviewed changes

andrii-kovalenko-ct approved these changes Jan 8, 2018

View reviewed changes

heshamMassoud added 6 commits January 8, 2018 17:53

#227: Use gradle for injecting token.

8420896

#227: Call gradle script from travis.

e6b6791

#227: Remove bash script.

676b960

#227: Remove .bak ignoring.

c3c1cd0

#227: Fix release notes.

095cd6c

#227: Adjust comment.

42f092c

heshamMassoud added 3 commits January 8, 2018 18:26

#227: Check if the version file contains the version placeholder and …

50d7d37

…fail build to catch failty releases.

#227: Removed unneeded backup step.

27de9e1

#227: Fix error message.

19e27c6

butenkor reviewed Jan 9, 2018

View reviewed changes

andrii-kovalenko-ct reviewed Jan 9, 2018

View reviewed changes

heshamMassoud added 4 commits January 9, 2018 10:45

#227: Change task name and remove pr check.

168c067

#227: Use new name of script.

ac9470f

#227: Use new name of script.

8379ce0

#227: execute gradle script inline in script.

62bddb4

#227: Ensure order of execution of setReleaseVersion.

dc27248

#227: Fix release notes.

b54b513

heshamMassoud merged commit b6a9f79 into master Jan 9, 2018

heshamMassoud deleted the 227-fix-version-header branch January 9, 2018 14:03

Conversation

heshamMassoud commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

More details

Relevant Issues

Todo

Update (FYI: @butenkor, @andrii-kovalenko-ct):

Uh oh!

codecov-io commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

butenkor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii-kovalenko-ct left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii-kovalenko-ct commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii-kovalenko-ct Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrii-kovalenko-ct Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrii-kovalenko-ct commented Jan 9, 2018

Uh oh!

andrii-kovalenko-ct commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heshamMassoud commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrii-kovalenko-ct commented Jan 9, 2018

Uh oh!

heshamMassoud commented Jan 8, 2018 •

edited

Loading

codecov-io commented Jan 8, 2018 •

edited

Loading

butenkor left a comment •

edited

Loading

andrii-kovalenko-ct left a comment •

edited

Loading

heshamMassoud Jan 9, 2018 •

edited

Loading

andrii-kovalenko-ct Jan 9, 2018 •

edited

Loading

andrii-kovalenko-ct Jan 9, 2018 •

edited

Loading

heshamMassoud Jan 9, 2018 •

edited

Loading

heshamMassoud commented Jan 9, 2018 •

edited

Loading

andrii-kovalenko-ct commented Jan 9, 2018 •

edited

Loading

heshamMassoud commented Jan 9, 2018 •

edited

Loading