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

fix: go version unification #88

Merged
merged 9 commits into from
Feb 11, 2025
Merged

fix: go version unification #88

merged 9 commits into from
Feb 11, 2025

Conversation

Mughees2001
Copy link
Contributor

@Mughees2001 Mughees2001 commented Feb 9, 2025

Closes #87

@Mughees2001 Mughees2001 marked this pull request as draft February 9, 2025 08:47
@Mughees2001 Mughees2001 marked this pull request as ready for review February 9, 2025 20:09
@Mughees2001
Copy link
Contributor Author

  • Used go.mod as the single source of truth for the Go version.
  • Updated Dockerfile to use a pre-built harpoon artifact instead of building it.
  • Modified Makefile to extract and pass the Go version from go.mod.
  • Updated GitHub Actions workflows (build-and-release.yaml, build.yaml, tests.yaml) to dynamically extract the Go version from go.mod.

@alegrey91
Copy link
Owner

  • Used go.mod as the single source of truth for the Go version.
  • Updated Dockerfile to use a pre-built harpoon artifact instead of building it.
  • Modified Makefile to extract and pass the Go version from go.mod.
  • Updated GitHub Actions workflows (build-and-release.yaml, build.yaml, tests.yaml) to dynamically extract the Go version from go.mod.

Hey @Mughees2001, thanks for your contribution, this was exactly what I needed.
Overall it looks good.
Just few comments:

  • Could you please add the also for the other Go versions at .github/workflows/tests.yaml (lines 62 and 119).
  • The Dockerfile is not using a pre-built harpoon artifact (you should basicallly remove the builder stage and COPY harpoon from outside, eg. COPY bin/harpoon .)

@Mughees2001
Copy link
Contributor Author

I have tried to fix the issues. Please let me know if anything else is required.

@alegrey91
Copy link
Owner

I have tried to fix the issues. Please let me know if anything else is required.

If you are referring to the pipeline, don't worry, it's a flaky test.
About the Dockerfile, you should remove all the lines before FROM debian:bookworm-slim.
Please test the whole flow to ensure this is working properly before pushing. Otherwise please restore the Dockerfile.

@Mughees2001
Copy link
Contributor Author

Mughees2001 commented Feb 11, 2025

I have restored the Dockerfile back. I couldn't test because my docker daemon wasn't working for some reason.

@alegrey91
Copy link
Owner

LGTM.
Thanks @Mughees2001 for your contribution :)
Thanks @ccoVeille for the suggested changes.

@alegrey91 alegrey91 merged commit 27b18be into alegrey91:main Feb 11, 2025
2 of 3 checks passed
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.

Unifying go version across files
3 participants