Script tool installation and linting #85

jarrpa · 2018-10-23T12:48:56Z

Move the linting and test commands out into of the Dockerfile into
scripts, using some existing scripting in the repo. This makes the
Dockerfile cleaner and allows the commands to be run outside the
container build for development purposes.

Signed-off-by: Jose A. Rivera jarrpa@redhat.com

jarrpa · 2018-10-23T15:04:58Z

I noticed that the second docker build (to extract the profiling data) was not caching any image layers from the first build. Restructured the build script a bit to fix that.

JohnStrunk · 2018-10-23T19:42:03Z

scripts/install-go-tools.sh

+    return
+  fi
+
+  echo "Installing gometalinter. Version: ${GLMVER}"


Typo: GLMVER ==> GMLVER

JohnStrunk · 2018-10-23T19:52:51Z

build.sh

@@ -11,25 +11,42 @@ RUN_TESTS=${RUN_TESTS:-1}
 VERSION="$(git describe --dirty --always --tags | sed 's/-/./2' | sed 's/-/./2')"
 BUILDDATE="$(date -u '+%Y-%m-%dT%H:%M:%S.%NZ')"

+GO_DEP_VERSION="${GO_DEP_VERSION}"


Suggestion: ${GO_DEP_VERSION:-latest}

It took me a while to track down where the dep version was actually set. It would be good to put it here w/ the other defaults.

Setting it to latest breaks the script. It expects either a version tag or empty string (as latest).

I guess I can see that here, https://github.com/gluster/gluster-csi-driver/pull/85/files#diff-b6e2d673cd08c78537e77f91e9e2272aR11, at the version check. Not ideal, but 🤷‍♂️

JohnStrunk · 2018-10-24T19:29:16Z

build.sh

@@ -11,25 +11,42 @@ RUN_TESTS=${RUN_TESTS:-1}
 VERSION="$(git describe --dirty --always --tags | sed 's/-/./2' | sed 's/-/./2')"
 BUILDDATE="$(date -u '+%Y-%m-%dT%H:%M:%S.%NZ')"

+GO_DEP_VERSION="${GO_DEP_VERSION}"


I guess I can see that here, https://github.com/gluster/gluster-csi-driver/pull/85/files#diff-b6e2d673cd08c78537e77f91e9e2272aR11, at the version check. Not ideal, but 🤷‍♂️

Madhu-1 · 2018-10-25T04:17:41Z

@humblec PTAL

humblec · 2018-10-25T12:21:54Z

build.sh

@@ -11,25 +11,42 @@ RUN_TESTS=${RUN_TESTS:-1}
 VERSION="$(git describe --dirty --always --tags | sed 's/-/./2' | sed 's/-/./2')"
 BUILDDATE="$(date -u '+%Y-%m-%dT%H:%M:%S.%NZ')"

+GO_DEP_VERSION="${GO_DEP_VERSION}"
+GO_METALINTER_VERSION="${GO_METALINTER_VERSION:-v2.0.11}"
+GO_METALINTER_THREADS=${GO_METALINTER_THREADS:-4}


Whats the deafult thread count ?

humblec · 2018-10-25T12:23:50Z

pkg/glusterfs/Dockerfile

-      GOPACKAGES="$(go list ./... | grep -v vendor | grep -v e2e)"; \
-      go test -covermode=count -coverprofile=/profile.cov $GOPACKAGES; \
-    }
+ARG GO_METALINTER_THREADS=1


Here the threads has been set to 1 and above on 4. Any specific reason for these 2 values ?

humblec · 2018-10-25T12:23:56Z

pkg/glusterfs/Dockerfile


+# Build executable
+RUN CGO_ENABLED=0 GOOS=linux go build -ldflags '-extldflags "-static"' -o /glusterfs-csi-driver cmd/glusterfs/main.go


The Output binary and source path can be variables.

jarrpa · 2018-10-25T15:16:51Z

Here the threads has been set to 1 and above on 4. Any specific reason for these 2 values ?

The GML default is "1", and I try to preserve defaults in the Dockerfile where possible. For our purposes, we've alwayd been running it with a thread count of 4, so I set that as the default in the build script.

The Output binary and source path can be variables.

Agreed, can do.

humblec · 2018-10-29T05:27:40Z

@Madhu-1 @JohnStrunk any further comments ?

JohnStrunk · 2018-10-29T15:42:21Z

pkg/glusterfs/Dockerfile

@@ -10,55 +10,60 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

+# The base image to use for the final images
+ARG FINAL_BASE=centos


Does this still build w/ buildah bud? I suspect this line and the construct on L56 will break it due to bugs/missing features.

Haven't tried. Any instructions on how to do that? Is it just run buildah bud from the root of the repo?

It's basically a replacement for docker build: docker build <stuff> ==> buildah bud <stuff>

So i should be able to test it by setting DOCKER_CMD="buildah bud"?

not quite that easy. That would produce buildah bud build <stuff>. You need to replace both "docker" and "build" parts of the command line. The other "stuff" should transfer as-is.

It looks like your original change already broke buildah, as it doesn't recognize --target. Should I try refactoring all this so that buildah runs?

Yeah... we need to keep buildah working. Of course I'm not sure how we're going to get the coverage data out w/o being able to refer to the intermediate image or splitting into multiple dockerfiles. And having multiple dockerfiles will prevent us from autobuilding on docker hub.
For now, I suppose pulling it forward into the final image then copying it out is good enough... it's small.

BTW, a quick search turned up the bug in buildah: containers/buildah#632

Now I'm running into a no build container available error that I can't find any diagnosis for online. Ideas?

@jarrpa is above issue resolved at ur end ?

Ah, yeah. Had to add docker.io to the base image names.

jarrpa · 2018-10-30T16:51:33Z

@JohnStrunk Added a commit to adjust my changes for buildah compatibility. PTAL

JohnStrunk · 2018-10-30T17:14:09Z

pkg/glusterfs/Dockerfile


-#-- Build phase
-FROM openshift/origin-release:golang-1.10 AS build
+FROM docker.io/openshift/origin-release:golang-1.10


FROM ... AS is supported. The following works for me:

$ cat Dockerfile FROM centos:7.5.1804 as base RUN echo "hello" > /h FROM centos:7.5.1804 as final COPY --from=base /h /h RUN cat /h

with

$ buildah bud . STEP 1: FROM centos:7.5.1804 Getting image source signatures Copying blob sha256:7dc0dca2b1516961d6b3200564049db0a6e0410b370bb2189e2efae0d368616f 71.23 MiB / 71.23 MiB [===================================================] 19s Copying config sha256:76d6bc25b8a5685072a1a99d9ac7c2e52dc3070081c872034a1889ca2d4bcf8c 2.15 KiB / 2.15 KiB [======================================================] 0s Writing manifest to image destination Storing signatures STEP 2: RUN echo "hello" > /h STEP 3: FROM centos:7.5.1804 STEP 4: COPY --from=base /h /h STEP 5: RUN cat /h hello STEP 6: COMMIT containers-storage:[vfs@/home/jstrunk/.local/share/containers/storage+/run/user/101587/run]@4a5769081c038b6ed9e8fe400078d068e773c6ed048ff485e648c2f1985113c2 Getting image source signatures Skipping fetch of repeat blob sha256:bcc97fbfc9e1a709f0eb78c1da59caeb65f43dc32cd5deeb12b8c1784e5b8237 Copying blob sha256:34e18c749bcf5457f2a2d4d36a3a0fce5e04f723c81be2dc38f37835256dbf27 169 B / 169 B [============================================================] 0s Copying config sha256:fcf270bec64248fd75cbe464ce99bfc0229c56ba2fdb7894a100532a97fd07cd 1.18 KiB / 1.18 KiB [======================================================] 0s Writing manifest to image destination Storing signatures --> 4a5769081c038b6ed9e8fe400078d068e773c6ed048ff485e648c2f1985113c2

Fair enough. I didn't see the point of keeping it if we're not making meaningful use of it, but I'll add it back in.

Previously, the second docker build would not actually use any images from the docker cache. If testing, build and tag the build container first, then build the final container. If not testing, only build once. Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

Move the linting and test commands out into of the Dockerfile into scripts, using some existing scripting in the repo. This makes the Dockerfile cleaner and allows the commands to be run outside the container build for development purposes. Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa · 2018-10-30T18:52:31Z

@JohnStrunk Updated per comments and to allow the CI to pass (needed to override the container entrypoint). PTAL

jarrpa · 2018-10-30T21:35:16Z

@humblec @Madhu-1 PTAL

humblec · 2018-11-02T05:20:27Z

pkg/glusterfs/Dockerfile

+RUN CGO_ENABLED=0 GOOS=linux go build -ldflags '-extldflags "-static"' -o /build/glusterfs-csi-driver cmd/glusterfs/main.go
+
+# Ensure the binary is statically linked
+RUN ldd /build/glusterfs-csi-driver | grep -q "not a dynamic executable"


Whats the benefit of this validation ?

I don't know, I didn't add it. Just keeping what was already there.

It guarantees the generated binary is statically linked, so there are no library dependencies that would need to be pulled into the final image.

humblec · 2018-11-02T05:22:04Z

pkg/glusterfs/Dockerfile


 #-- Final container
-FROM centos:7.5.1804
+
+FROM docker.io/centos:7.5.1804 as final


why cant we use latest tag here ?

I don't know, I didn't add it. Just keeping what was already there.

Because if you use latest, you don't get a repeatable container image.

humblec · 2018-11-02T05:24:06Z

Also scripts/pre-commit.sh → scripts/lint-text.sh is it required ? pre-commit looks better, isnt it ?

jarrpa · 2018-11-02T11:54:56Z

I don't think pre-commit looks better. It's vague and does not describe what the script does. It made sense when there were more things in it, but now it only does one thing, generally. Other things like the go linting would also technically count as "pre-commit" actions, but have always been in their own files.

humblec · 2018-11-05T10:17:23Z

This is good to go. @JohnStrunk @Madhu-1 any final comments ?

jarrpa requested a review from JohnStrunk October 23, 2018 12:48

ghost assigned jarrpa Oct 23, 2018

ghost added the in progress label Oct 23, 2018

Madhu-1 requested review from Madhu-1, humblec and JohnStrunk and removed request for JohnStrunk October 23, 2018 12:57

jarrpa force-pushed the build-scripts branch 4 times, most recently from 898d44d to a91d08d Compare October 23, 2018 15:04

JohnStrunk reviewed Oct 23, 2018

View reviewed changes

JohnStrunk previously approved these changes Oct 24, 2018

View reviewed changes

jarrpa dismissed JohnStrunk’s stale review via 3ae1ef3 October 24, 2018 19:36

jarrpa force-pushed the build-scripts branch from a91d08d to 3ae1ef3 Compare October 24, 2018 19:36

JohnStrunk previously approved these changes Oct 24, 2018

View reviewed changes

Madhu-1 previously approved these changes Oct 25, 2018

View reviewed changes

humblec reviewed Oct 25, 2018

View reviewed changes

jarrpa dismissed stale reviews from Madhu-1 and JohnStrunk via 1485ca2 October 25, 2018 17:45

jarrpa force-pushed the build-scripts branch 3 times, most recently from 460832f to 96f6550 Compare October 25, 2018 19:41

Madhu-1 previously approved these changes Oct 29, 2018

View reviewed changes

JohnStrunk reviewed Oct 29, 2018

View reviewed changes

jarrpa dismissed Madhu-1’s stale review via 701c939 October 30, 2018 16:51

JohnStrunk reviewed Oct 30, 2018

View reviewed changes

jarrpa added 5 commits October 30, 2018 12:50

Display Docker version on all builds

0a6d7fd

Tweak build logic / labeling

7c95d9e

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

Add more build parameters

fde8c08

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa force-pushed the build-scripts branch from 701c939 to db0c107 Compare October 30, 2018 17:51

Fix for buildah

34c7af9

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>

jarrpa force-pushed the build-scripts branch from db0c107 to 34c7af9 Compare October 30, 2018 18:45

JohnStrunk approved these changes Oct 30, 2018

View reviewed changes

humblec reviewed Nov 2, 2018

View reviewed changes

JohnStrunk merged commit de19709 into gluster:master Nov 5, 2018

ghost removed the in progress label Nov 5, 2018


		# Build executable
		RUN CGO_ENABLED=0 GOOS=linux go build -ldflags '-extldflags "-static"' -o /glusterfs-csi-driver cmd/glusterfs/main.go

Script tool installation and linting #85

Script tool installation and linting #85

Conversation

jarrpa commented Oct 23, 2018

jarrpa commented Oct 23, 2018

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

Madhu-1 commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrpa commented Oct 25, 2018

humblec commented Oct 29, 2018

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

jarrpa commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrpa commented Oct 30, 2018

jarrpa commented Oct 30, 2018

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

humblec commented Nov 2, 2018

jarrpa commented Nov 2, 2018

humblec commented Nov 5, 2018