[GPII-3841]: Add display_universal_image_info task #344

natarajaya · 2019-04-04T03:43:02Z

This should be helpful for developers, while we think on better long-term approach.

stepanstipl

Seems useful, good idea 👍

shared/rakefiles/entrypoint.rake

stepanstipl · 2019-04-04T12:57:36Z

shared/rakefiles/entrypoint.rake

+  sh "#{@exekube_cmd} sh -c ' \
+    UNIVERSAL_CI_URL=\"https://ci.gpii.net/\";
+    UNIVERSAL_REPO=\"https://github.com/gpii/universal\";
+    RELEASE_JOB_URL=\"$UNIVERSAL_CI_URL/job/docker-gpii-universal-master-release\";


Can we put braces around variables here: ${UNIVERSAL}

Why do you think they are needed here?

I can think of only one common case when variable expansion braces are needed – var="${another_var}and_not_a_var". Well, another not so common case maybe working with arrays or some string manipulations. Otherwise they contribute to nothing, imo.

I guess this is a personal taste, but specially in the cases like "$UNIVERSAL_CI_URL/job/... I would use them, as it's not very obvious imho when this will be treated as a variable ${UNIVERSAL}, ${UNIVERSAL_CI_URL} or variable ${UNIVERSAL_CI_URL/job/...} - depending on which characters exaclty given shell considers a word separators. Adding {} make the intention very clear.

Using capital case for variables has the same effect on code readability as using braces with low case variables. Both is probably overkill :) @mrtyler @amatas What do you guys think on this topic?

Using capital case for variables has the same effect on code readability as using braces with low case variables.

Not sure I agree with this for humans, but I disagree for computers -- braces have special meaning to the parser whereas capitalized variable names do not.

Both is probably overkill

Why?

What do you guys think on this topic?

I think Stepan's comment is nitpicky and I would not have suggested it. However, I understand his motivation and I would make his suggested change if I had received it on one of my PRs.

Longer-term, I think the solution to debates about code style is to automate code style checking and/or fixing (a la terraform fmt). I don't think we're ready to deal with that, especially since we have a fair amount of shell scripts mixed with terraform code, which complicates the tooling. (I've mentioned this before, e.g. #93 (comment).)

But anyway, here is what I got from dropping this shell code into shellcheck:

(EDIT: Oops, forgot to fix escaped quotes the first time. Output below is updated.)

In sergey.sh line 18: RELEASE_BUILD_LIMIT=$(($RELEASE_BUILD-$LOOKUP_BUILDS)); ^-- SC2004: $/${} is unnecessary on arithmetic variables. ^-- SC2004: $/${} is unnecessary on arithmetic variables. In sergey.sh line 20: SHA_FOUND=$(curl $RELEASE_JOB_URL/$RELEASE_BUILD/consoleText 2> /dev/null | grep -so "$PREFERENCES_IMAGE_SHA" || true); ^-- SC2086: Double quote to prevent globbing and word splitting. In sergey.sh line 22: UPSTREAM_JOB_NUMBER=$(curl $RELEASE_JOB_URL/$RELEASE_BUILD/api/json 2> /dev/null | jq -r ".actions[] | select (.causes[0].upstreamBuild != null) | .causes[0].upstreamBuild"); ^-- SC2086: Double quote to prevent globbing and word splitting. In sergey.sh line 23: GITHUB_LINK="$UNIVERSAL_REPO/commit/$(curl $UPSTREAM_JOB_URL/$UPSTREAM_JOB_NUMBER/api/json 2> /dev/null | jq -r ".actions[] | select (.lastBuiltRevision.SHA1 != null) | .lastBuiltRevision.SHA1")"; ^-- SC2086: Double quote to prevent globbing and word splitting. In sergey.sh line 32: RELEASE_BUILD=$(($RELEASE_BUILD-1)); ^-- SC2004: $/${} is unnecessary on arithmetic variables.

@mrtyler Thanks for review!

braces have special meaning to the parser whereas capitalized variable names do not

Yes, I think we all know that here, context of original discussion was limited to affect of braces on code readability, since code works just fine with them or without them.

Longer-term, I think the solution to debates about code style is to automate code style checking and/or fixing (a la terraform fmt). I don't think we're ready to deal with that

Agree and agree.

But anyway, here is what I got from dropping this shell code into shellcheck

Cool, let me address some of those warnings!

stepanstipl · 2019-04-04T13:03:06Z

shared/rakefiles/entrypoint.rake

@@ -108,6 +108,55 @@ task :display_cluster_info => [:set_vars] do
  puts
 end

+desc "Display gpii/universal image SHA and link to GitHub commit that triggered the build"
+task :display_universal_image_info => [:set_vars] do
+  sh "#{@exekube_cmd} sh -c ' \


Consider setting -e flag to stop on errors? Or add some error handling, as there's none for most parts of this script - curl calls, an unexpected structure of outputs... (I think -e might be easy way out in this case ;))

There are condition checks to handle most common failures (unable to get data from k8s, unable to generate commit link), but yeah, setting -e flag makes sense.

Actually nevermind, it is not going to work properly with set -e.

Fair enough, I think that it's generally dangerous to rely on output of given commands, not even checking for return codes. Like relying on empty output of curl | jq in a while condition.

I see your point, but I think since this is just utility task, and nothing terrible going to happen even when it fails, current level of error handling is good enough.

amatas · 2019-04-04T16:24:23Z

Good idea @natarajaya of using the CI to match the images with the commits. But I think that something is wrong here. I tried this code and I got:

Deployed gpii/universal image SHA:
sha256:eab9307f4b1549077c97646848e54b49fc6ceed42d98185882e2258b409f0770

Commit to gpii/universal that triggered image build:
https://github.com/gpii/universal/commit/02584b737cf6f05175402fd46484616aee706937

If I go to the last job: https://ci.gpii.net/job/docker-gpii-universal-master/158/
seems that the image SHA is correct, but the commit SHA is wrong.

My guess is that the script is using the wrong build number. Bear in mind that the build number can be different in between the jobs of a multijob: the parent job number is 158 but the release job is 156.

natarajaya · 2019-04-04T20:31:05Z

My guess is that the script is using the wrong build number. Bear in mind that the build number can be different in between the jobs of a multijob: the parent job number is 158 but the release job is 156.

Thanks @amatas, nice catch! This is fixed now. I also followed your advice and added this task to CI for staging and prod.

mrtyler

LGTM. This looks more useful than the documentation I was writing to describe the process. :)

mrtyler · 2019-04-04T04:16:38Z

shared/rakefiles/entrypoint.rake

+      echo
+      echo \"Unable to retrieve data from K8s cluster!\";
+      echo \"This is most likely because you are not authenticated!\";
+      echo \"Try running \\\`rake sh[\\\"rake configure_kubectl\\\"]\\\` and then try again.\";


Can we use rake dependencies so that this happens automatically?

Sure, let me move the task to xk_util and make an alias similarly to display_cluster_state.

mrtyler · 2019-04-04T22:07:45Z

shared/rakefiles/entrypoint.rake

+    fi
+
+    echo
+    echo \"Deployed gpii/universal image SHA:\";


I would mention that it's the gpii/universal image in use by preferences, in case there happens to be a discrepancy between preferences and other components that use gpii/universal.

Makes sense!

stepanstipl · 2019-04-05T15:12:51Z

I also followed your advice and added this task to CI for staging and prod.

What will happen when this task fails, will it fail the build? If so, it seems quite risky to make our pipeline dependant on availability of GPII Jenkins server.

natarajaya · 2019-04-05T22:06:01Z

What will happen when this task fails, will it fail the build?

This is fixed.

mrtyler · 2019-04-08T14:13:31Z

shared/rakefiles/xk_util.rake

+
+    PREFERENCES_IMAGE_SHA=$(kubectl -n gpii get deployment preferences -o json 2> /dev/null | jq -r \".spec.template.spec.containers[0].image\" | grep -o \"sha256:.*\");
+    if [ \"$?\" != \"0\" ]; then
+      echo


Since I'm looking at this code again, why do most lines end with ; but some do not?

sh collapses everything into a single string, ; are required to separate commands that need it from each other. Alternative would be to escape newlines. I think this is another sign that bash code does not belong to rake and we should do something about it in future.

If this is true, then does line 143 combine with line 144 and become echo exit 1 (which will not exit)?

I guess if it worked in testing then the inconsistency isn't a big deal.

mrtyler · 2019-04-08T14:16:59Z

shared/rakefiles/xk_util.rake

+    echo \"Preferences image SHA:\";
+    echo \"$PREFERENCES_IMAGE_SHA\";
+    RELEASE_BUILD=$(curl $RELEASE_JOB_URL/lastBuild/api/json 2> /dev/null | jq -r \".id\");
+    RELEASE_BUILD_LIMIT=$(($RELEASE_BUILD-$LOOKUP_BUILDS));


I would add spaces around - to make it clear this is arithmetic subtraction, not joining strings together.

Yeah, shellcheck suggests the same.

mrtyler · 2019-04-08T14:18:33Z

shared/rakefiles/xk_util.rake

+      SHA_FOUND=$(curl $RELEASE_JOB_URL/$RELEASE_BUILD/consoleText 2> /dev/null | grep -so \"$PREFERENCES_IMAGE_SHA\" || true);
+      if [ \"$SHA_FOUND\" == \"$PREFERENCES_IMAGE_SHA\" ]; then
+        UPSTREAM_JOB_NUMBER=$(curl $RELEASE_JOB_URL/$RELEASE_BUILD/api/json 2> /dev/null | jq -r \".actions[] | select (.causes[0].upstreamBuild != null) | .causes[0].upstreamBuild\");
+        GITHUB_LINK=\"$UNIVERSAL_REPO/commit/$(curl $UPSTREAM_JOB_URL/$UPSTREAM_JOB_NUMBER/api/json 2> /dev/null | jq -r \".actions[] | select (.lastBuiltRevision.SHA1 != null) | .lastBuiltRevision.SHA1\")\";


Might want to use curl -f here.

Makes sense.

stepanstipl · 2019-04-08T15:41:44Z

What will happen when this task fails, will it fail the build?

This is fixed.

lgtm

natarajaya added 3 commits April 3, 2019 20:31

Add display_universal_image_info task

3842c24

Update README

374ae7b

Remove unnecessary escapes

2a084af

natarajaya self-assigned this Apr 4, 2019

Update comment

7370ebb

stepanstipl approved these changes Apr 4, 2019

View reviewed changes

natarajaya added 3 commits April 4, 2019 13:05

Address issues from PR comments

93eae58

Use verbose: false

4b0d0a4

Add display_universal_image_info into CI config for stg, prd

a7b50eb

natarajaya changed the title ~~Add display_universal_image_info task~~ [GPII-3841]: Add display_universal_image_info task Apr 4, 2019

Remove set -e

64baddd

mrtyler reviewed Apr 4, 2019

View reviewed changes

natarajaya added 2 commits April 4, 2019 15:58

Move task to xk_util, minor updates

62ceb76

One more fix

cb559b6

Do not fail the build if display_universal_image_info fails

616580d

mrtyler reviewed Apr 8, 2019

View reviewed changes

Address shellcheck warnings

b2e513c

natarajaya merged commit 95bf103 into gpii-ops:master Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GPII-3841]: Add display_universal_image_info task #344

[GPII-3841]: Add display_universal_image_info task #344

natarajaya commented Apr 4, 2019

stepanstipl left a comment

stepanstipl Apr 4, 2019

natarajaya Apr 4, 2019 •

edited

stepanstipl Apr 5, 2019 •

edited

natarajaya Apr 5, 2019

mrtyler Apr 8, 2019 •

edited

natarajaya Apr 8, 2019

stepanstipl Apr 4, 2019

natarajaya Apr 4, 2019

natarajaya Apr 4, 2019

stepanstipl Apr 5, 2019

natarajaya Apr 5, 2019

amatas commented Apr 4, 2019

natarajaya commented Apr 4, 2019

mrtyler left a comment

mrtyler Apr 4, 2019

natarajaya Apr 4, 2019

mrtyler Apr 4, 2019

natarajaya Apr 4, 2019

stepanstipl commented Apr 5, 2019

natarajaya commented Apr 5, 2019

mrtyler Apr 8, 2019

natarajaya Apr 8, 2019

mrtyler Apr 9, 2019

mrtyler Apr 8, 2019

natarajaya Apr 8, 2019

mrtyler Apr 8, 2019

natarajaya Apr 8, 2019

stepanstipl commented Apr 8, 2019

[GPII-3841]: Add display_universal_image_info task #344

[GPII-3841]: Add display_universal_image_info task #344

Conversation

natarajaya commented Apr 4, 2019

stepanstipl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natarajaya Apr 4, 2019 • edited

Choose a reason for hiding this comment

stepanstipl Apr 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrtyler Apr 8, 2019 • edited

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

amatas commented Apr 4, 2019

natarajaya commented Apr 4, 2019

mrtyler 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

stepanstipl commented Apr 5, 2019

natarajaya commented Apr 5, 2019

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

stepanstipl commented Apr 8, 2019

natarajaya Apr 4, 2019 •

edited

stepanstipl Apr 5, 2019 •

edited

mrtyler Apr 8, 2019 •

edited