-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Enable Go build tags #3950
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
Enable Go build tags #3950
Conversation
This enables go build tags so the GCS and OSS driver support is available in the binary distributed via the image build by Dockerfile. This led to quite a few fixes in the GCS and OSS packages raised as warning by golang-ci linter. Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
Codecov ReportPatch and project coverage have no change.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #3950 +/- ##
=======================================
Coverage 55.88% 55.88%
=======================================
Files 108 108
Lines 10949 10949
=======================================
Hits 6119 6119
Misses 4145 4145
Partials 685 685 ☔ View full report in Codecov by Sentry. |
Some of these changes are refactors, which is fine, but there's also things like returning new errors and adding timeouts. I think these are good changes, but I think those should rather be in a separate PR for the sake of git history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things, but I won't block on them. 🚀
//go:build include_gcs | ||
// +build include_gcs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to maintain support for the pre 1.17 style build tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had this discussion a while back and decided to stick with them, though at this point we're no longer building images for that version of Go so we should revisit 🤔 CC: @thaJeztah
Yeah, these were needed to keep the linter quiet I'm afraid. There are some deprecation notes in Googles cloud SDK (e.g. https://pkg.go.dev/cloud.google.com/go/storage#Writer.CloseWithError) that didn't surface during the original GCS rework PR because the linter never ran due to the missing build tags in the lint action. |
PTAL @squizzi |
Perhaps a separate commit at minimum as part of this PR would be a good idea. |
If I do split these changes one of those commits will ultimately fail the build - as I said earlier, these would have been caught during the original GCS PR if the build tags had been enabled |
No worries |
It would appear we are missing the Go build tags so the binary packaged in the image built from the source in this repo would miss support for some storage drivers causing breakages to end users trying to use them.
This PR enables Go build tags so the GCS and OSS driver support is available in the binary distributed via the image build by Dockerfile.
This led to quite a few fixes in the GCS and OSS packages raised as warnings by
golang-ci
linter.