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.

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

update Go version, alpine, golangci and gotestsum #206

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

Conversation

converge
Copy link
Contributor

Signed-off-by: João Vanzuita joaovanzuita@me.com

- What I did

Updated
Go version to 1.18
Docker CLI to 20.10.12
ALPINE Docker image to 3.15.4
GoLang Lint to 1.45.2
Go Test Sum to 1.8.0

- How I did it

Updating software version and testing it manually on a Mac M1.

- How to verify it

Build the project without cache.

- Description for the changelog

Update Go Lang to 1.8 and update overall dependencies to the most recent versions.

p.s: before we merge, it would be nice to test on Linux and Windows.

- A picture of a cute animal (not mandatory)

Signed-off-by: João Vanzuita <joaovanzuita@me.com>
@silvin-lubecki
Copy link
Collaborator

The linter is failing:

#13 0.745 go: added github.com/kunalkushwaha/ltag v0.2.3
#13 0.748 ERROR: ltag not found. Install with:
#13 0.748     go get -u github.com/kunalkushwaha/ltag

We should fix it with

$ go install github.com/kunalkushwaha/ltag

Can you handle it @converge ?

Signed-off-by: João Vanzuita <joaovanzuita@me.com>
@converge
Copy link
Contributor Author

The linter is failing:

#13 0.745 go: added github.com/kunalkushwaha/ltag v0.2.3
#13 0.748 ERROR: ltag not found. Install with:
#13 0.748     go get -u github.com/kunalkushwaha/ltag

We should fix it with

$ go install github.com/kunalkushwaha/ltag

Can you handle it @converge ?

sure thing! go install ...@version did the trick.

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 👍

vars.mk Outdated
GOLANGCI_LINT_VERSION=v1.33.0-alpine
GOTESTSUM_VERSION=0.6.0
GO_VERSION=1.18-alpine
CLI_VERSION=20.10.12
Copy link
Member

Choose a reason for hiding this comment

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

While we're updating; could you make this 20.10.14 (which is latest)? (same for the Dockerfile)

Signed-off-by: João Vanzuita <joaovanzuita@me.com>
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
@converge converge requested a review from thaJeztah April 14, 2022 20:53
GOTESTSUM_VERSION=0.6.0
GO_VERSION=1.18-alpine
CLI_VERSION=20.10.14
ALPINE_VERSION=3.15.4

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants