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.

Sign up for GitHub

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

Jump to bottom

update Go version, alpine, golangci and gotestsum #206

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

update Go version, alpine, golangci and gotestsum #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19910c2
update Go version, alpine, golangci and gotestsum
converge Apr 13, 2022
820b0ac
call go install instead of go get to use ltag
converge Apr 14, 2022
401204d
update docker cli
converge Apr 14, 2022
3ebe0b9
remove unecessary dockerfile arg definition
converge Apr 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
# limitations under the License.


ARG GO_VERSION=1.16.3-alpine
ARG CLI_VERSION=20.10.2
ARG ALPINE_VERSION=3.12.2
ARG GOLANGCI_LINT_VERSION=v1.33.0-alpine
ARG GO_VERSION
ARG CLI_VERSION
ARG ALPINE_VERSION
ARG GOLANGCI_LINT_VERSION

####
# BUILDER
Expand Down Expand Up @@ -58,7 +58,8 @@ RUN --mount=type=cache,target=/root/.cache/go-build \
FROM builder AS validate-headers
RUN --mount=type=cache,target=/root/.cache/go-build \
--mount=type=cache,target=/go/pkg \
go get -u github.com/kunalkushwaha/ltag && ./scripts/validate/fileheader
go install github.com/kunalkushwaha/ltag@v0.2.3 && ./scripts/validate/fileheader


####
# CHECK GO MOD
Expand Down
10 changes: 5 additions & 5 deletions vars.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
# limitations under the License.

# Pinned Versions
GO_VERSION=1.16.3-alpine
CLI_VERSION=20.10.2
ALPINE_VERSION=3.12.2
GOLANGCI_LINT_VERSION=v1.33.0-alpine
GOTESTSUM_VERSION=0.6.0
GO_VERSION=1.18-alpine
Copy link
Member

Choose a reason for hiding this comment

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

@silvin-lubecki do you prefer to keep the go version pinned to minor releases? (1.18.1 currently)? (same for the Dockerfile probably).

Slightly confused why we define defaults for them both as a make var and as a default var in the Dockerfile though; I think if we'd change the Makefile to remove the =$(VARIABLE), the build-args should take the value from the environment (if set), and otherwise take the default from the Dockerfile, i.e., something like:

BUILD_ARGS:=--build-arg GO_VERSION \
    --build-arg CLI_VERSION \
    --build-arg ALPINE_VERSION \
    --build-arg GOLANGCI_LINT_VERSION) \
    --build-arg TAG_NAME \
    --build-arg GOTESTSUM_VERSION \
    --build-arg BINARY_NAME \
    --build-arg BINARY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I can jump in.. keep up with 1.18 (1.18.x) would be nice, since it uses the most recent version, and the minor changes from Go are usually security updates.

Regarding point 2, you're right, we were setting the dependencies at the Makefile and var.mk file. I updated the Dockerfile to only read the ARG values from the Makefile (that sets the ARG values). WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep the defaults in the Dockerfile if that works, that way the Dockerfile itself has usable defaults, which allows it to be built without having to pass specific build args.

Not near my computer to try if that works though ☺️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch @thaJeztah 👍

CLI_VERSION=20.10.14
ALPINE_VERSION=3.15.4
Copy link

Choose a reason for hiding this comment

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

Let's go to 3.17 now

GOLANGCI_LINT_VERSION=v1.45.2-alpine
GOTESTSUM_VERSION=1.8.0

GOOS?=$(shell go env GOOS)
GOARCH?=$(shell go env GOARCH)
Expand Down