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

Lint, gofmt #207

Merged
merged 2 commits into from
Nov 5, 2023
Merged

Lint, gofmt #207

merged 2 commits into from
Nov 5, 2023

Conversation

taigrr
Copy link
Contributor

@taigrr taigrr commented Nov 2, 2023

Updates code to remove deprecation notices and update dependencies accordingly.
Runs gofmt, golint, etc. to apply DeMorgan's law, and use go code simplifier.
Removes dependency on deprecated ioutil package.

Checklist

  • Added unit / integration tests for windows, macOS and Linux?
  • Added a changelog entry in CHANGELOG.md?
  • Updated the documentation (README.md, docs)?
  • Does your change work on Linux, Windows and macOS?

@taigrr
Copy link
Contributor Author

taigrr commented Nov 2, 2023

Due to vendoring, this looks like a >250K line diff, but I've kept my commits atomic. Hopefully it's not too painful to review.

@dylanhitt
Copy link
Member

Looking at the tests it doesn't look to be building.

@taigrr
Copy link
Contributor Author

taigrr commented Nov 2, 2023

Thanks, I couldn't see the failures before but they're caused by docker using strings.Cut, which wasn't introduced to stdlib until go 1.18. commander + circleci was set to 1.17. Debian stable supports 1.19, so I've updated the go version numbers to 1.19.

@dylanhitt
Copy link
Member

Seems they changed their image names: https://circleci.com/developer/images/image/cimg/go 😄

@taigrr taigrr force-pushed the lint-gofmt branch 2 times, most recently from bfcaca4 to 7e14839 Compare November 2, 2023 20:11
@taigrr
Copy link
Contributor Author

taigrr commented Nov 2, 2023

Looks like first, curl was refusing to run, and now the test-reporter is refusing to run. Is there a chance you might be able to look into the test-reporter failure?

Very likely, this image is significantly different from the prior one and is missing a lot of functionality that the project was using. Might be better to just use the standard golang image.

@taigrr
Copy link
Contributor Author

taigrr commented Nov 2, 2023

the /bin/sh failure is what's got me scratching my head.

+ docker run --rm -v /var/run/docker.sock:/var/run/docker.sock --network commander_test --name commander-integration-go-test --env CC_TEST_REPORTER_ID=**************************************************************** commander-int-test make integration-linux
INFO: Starting build build
go build cmd/commander/*
make: /bin/sh: Operation not permitted
make: *** [Makefile:22: build] Error 127
make: *** [Makefile:85: integration-linux-dockerized] Error 2

Exited with code exit status 2
CircleCI received exit code 2```

@taigrr
Copy link
Contributor Author

taigrr commented Nov 2, 2023

With that reversion, my PR and your master 1.21 bump should match.

@dylanhitt
Copy link
Member

Also this...

Digest: sha256:b113af1e8b06f06a18ad41a6b331646dff587d7a4cf740f4852d16c49ed8ad73
Status: Downloaded newer image for golang:1.21
root@1a751dc82d21:/go# curl
curl: try 'curl --help' or 'curl --manual' for more information
root@1a751dc82d21:/go#

curl is definitely in there... In the past I have problem swith circle and it's transient errors...

@dylanhitt
Copy link
Member

Alright so something about make and latest debian, when executing in circle ci, only when executing from a docker run, on a thursday in November, does it not work 🥲

But yeah I set the test image back to debian bullseye. I don't really feel like opening the issues for all the places cause we're probably not gonna get anywhere. Hopefully some big corporation hits a wall with this 🤷. Probably will just move to github actions when the time comes.

@taigrr
Copy link
Contributor Author

taigrr commented Nov 3, 2023

@dylanhitt it's now friday UTC and the tests have passed. 🤣

(for real though, it was definitely the image.)

@taigrr
Copy link
Contributor Author

taigrr commented Nov 3, 2023

Hey, don't mean to double tag but the github outage lost all my pr notifications so in case yours are gone too, I'm bumping this. Feel free to ignore if you're busy.

Copy link
Member

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

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

👍

@dylanhitt
Copy link
Member

@taigrr can you squash this into one commit.

Thank you

automatic gofmt

chore: update go deps, fix broken docker api

run gofmt and linting to remove deprecation warnings

bump go version to latest available to debian

https://discuss.circleci.com/t/go-lang-docker-image-circleci-golang-1-19-is-missing/44961

fix golang => go for new circleci images

remove curl dependency

chore: bump go version to 1.21

Revert "remove curl dependency"

This reverts commit 7e14839.

chore: forgot test dockerfile
@taigrr
Copy link
Contributor Author

taigrr commented Nov 5, 2023

Whoops, my indexes were off by one. Is this ok?

@taigrr
Copy link
Contributor Author

taigrr commented Nov 5, 2023

Should be able to do a squash from the github UI now as well

@dylanhitt dylanhitt merged commit 72359e9 into commander-cli:master Nov 5, 2023
@taigrr
Copy link
Contributor Author

taigrr commented Nov 5, 2023

Looks like not all the integration tests run in the PR that run on master

@dylanhitt
Copy link
Member

Yeah, the windows test burn through circle ci minutes. Feel free to fix them or I'll do it later.

@taigrr
Copy link
Contributor Author

taigrr commented Nov 5, 2023

I think just need to update the windows version tag from v2.1.0 to v5.0.0, but I can't trigger the CI to test without a merge so it's best for you to do it

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