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

chore: installation for Go 1.18 #350

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

mrkagelui
Copy link
Contributor

addresses #349

@mrkagelui mrkagelui requested a review from a team as a code owner June 15, 2022 14:13
@asaf-erlich
Copy link
Contributor

Would it be too much to ask you to add 1.17.x and 1.18.x to the github action build matrix here: https://github.com/segmentio/chamber/blob/master/.github/workflows/build.yml#L14

@mrkagelui
Copy link
Contributor Author

Would it be too much to ask you to add 1.17.x and 1.18.x to the github action build matrix here: https://github.com/segmentio/chamber/blob/master/.github/workflows/build.yml#L14

certainly, if that's ok

@mrkagelui
Copy link
Contributor Author

Would it be too much to ask you to add 1.17.x and 1.18.x to the github action build matrix here: https://github.com/segmentio/chamber/blob/master/.github/workflows/build.yml#L14

how about test and dist though, should they be added or are they not supported?

@mrkagelui
Copy link
Contributor Author

I'll just add and ask for forgiveness 😆

@asaf-erlich
Copy link
Contributor

how about test and dist though, should they be added or are they not supported?

My gut tells me to update those as well. I don't see any reason why they should fail. But I don't want to throw unnecessary work on you. Feel free to try it but if it fails don't worry about it.

@mrkagelui
Copy link
Contributor Author

how about test and dist though, should they be added or are they not supported?

My gut tells me to update those as well. I don't see any reason why they should fail. But I don't want to throw unnecessary work on you. Feel free to try it but if it fails don't worry about it.

done, thanks for attending to this! I'm kinda surprised no one has encountered this yet.

@asaf-erlich
Copy link
Contributor

I'm not really sure why the go 1.18.x fails to find chamber after "installing" with go get, other than maybe go get doesn't work anymore in the same way?

Run go get -v . && chamber version
go: downloading github.com/magiconair/properties v1.8.0
go: downloading github.com/pkg/errors v0.9.1
go: downloading github.com/spf13/cobra v0.0.3
go: downloading golang.org/x/sys v0.0.0-20220[6](https://github.com/segmentio/chamber/runs/6902029207?check_suite_focus=true#step:4:7)14162138-6c1b26c55098
go: downloading gopkg.in/segmentio/analytics-go.v3 v3.0.1
go: downloading gopkg.in/yaml.v3 v3.0.0-20200506231410-2ff61e1afc86
go: downloading github.com/aws/aws-sdk-go v1.38.22
go: downloading github.com/inconshreveable/mousetrap v1.0.0
go: downloading github.com/spf13/pflag v1.0.2
go: downloading github.com/segmentio/backo-go v0.0.0-20160424052352-2042[7](https://github.com/segmentio/chamber/runs/6902029207?check_suite_focus=true#step:4:8)4ad699c
go: downloading github.com/xtgo/uuid v0.0.0-20140[8](https://github.com/segmentio/chamber/runs/6902029207?check_suite_focus=true#step:4:9)04021211-a0b114877d4c
go: downloading github.com/jmespath/go-jmespath v0.4.0
/home/runner/work/_temp/bcb03eb1-f7bd-420[9](https://github.com/segmentio/chamber/runs/6902029207?check_suite_focus=true#step:4:10)-9aa3-ff0768fa483d.sh: line 1: chamber: command not found
Error: Process completed with exit code [12](https://github.com/segmentio/chamber/runs/6902029207?check_suite_focus=true#step:4:13)7.

@asaf-erlich
Copy link
Contributor

Yup: https://go.dev/doc/go-get-install-deprecation:

Starting in Go 1.17, installing executables with go get is deprecated. go install may be used instead.

In Go 1.18, go get will no longer build packages; it will only be used to add, update, or remove dependencies in go.mod. Specifically, go get will always act as if the -d flag were enabled.

@asaf-erlich
Copy link
Contributor

I don't want to block your PR, you have a couple of options:

  1. Just drop 1.18.x for now for the install.
  2. Change it to use go install and hope that's backwards compatible for all the versions?

@mrkagelui
Copy link
Contributor Author

Yup: https://go.dev/doc/go-get-install-deprecation:

Starting in Go 1.17, installing executables with go get is deprecated. go install may be used instead.

In Go 1.18, go get will no longer build packages; it will only be used to add, update, or remove dependencies in go.mod. Specifically, go get will always act as if the -d flag were enabled.

hey sorry for the late reply, yeah that's exactly what this PR is trying to resolve, let me check later how to fix the pipeline

@mrkagelui
Copy link
Contributor Author

I don't have other go version installed locally, so here's what I'm gonna try:

  1. shoot in the dark and go with install;
  2. if that fails, I'm gonna split the job into two, >1.17 and older, wdyt?

@mrkagelui
Copy link
Contributor Author

what do I know? it works!

@asaf-erlich
Copy link
Contributor

Alright everything passed except that dockerhub login which I think was broken before or is at least flaky, I'll approve the PR.

@mrkagelui
Copy link
Contributor Author

cool, looking forward to the new version!

@asaf-erlich
Copy link
Contributor

cc @rikez do you know why the docker hub login github action task fails? I remember it happened before for you

@rikez
Copy link
Contributor

rikez commented Jun 15, 2022

@asaf-erlich It is failing because secrets aren't available in repo forks. Basically, the job is attempting to log in to Docker Registry using empty login and password values.

This docker step should be removed from feature branches. Let me open a PR to remove it. #351

@asaf-erlich asaf-erlich merged commit 61b73c4 into segmentio:master Jun 15, 2022
@mrkagelui mrkagelui deleted the chore-installation-1.18 branch June 16, 2022 03:21
@mrkagelui
Copy link
Contributor Author

@asaf-erlich would be great if we can get a new version of this so that installation is easier

@asaf-erlich
Copy link
Contributor

@mrkagelui I will create a backlog ticket for the team to prioritize a new release.

@mrkagelui
Copy link
Contributor Author

@mrkagelui I will create a backlog ticket for the team to prioritize a new release.

awesome, thank you and the team for this great tool, lots of ❤️

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.

3 participants