Skip to content

Conversation

milosgajdos
Copy link
Member

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.

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-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (71a6c56) 55.88% compared to head (6b388b1) 55.88%.

❗ 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@deleteriousEffect
Copy link
Member

This led to quite a few fixes in the GCS and OSS packages raised as warnings by golang-ci linter.

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.

Copy link
Member

@deleteriousEffect deleteriousEffect left a 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. 🚀

Comment on lines +1 to +2
//go:build include_gcs
// +build include_gcs
Copy link
Member

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?

Copy link
Member Author

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

@milosgajdos
Copy link
Member Author

but there's also things like returning new errors and adding timeouts

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.

@milosgajdos
Copy link
Member Author

PTAL @squizzi

@squizzi
Copy link
Collaborator

squizzi commented Jun 29, 2023

but I think those should rather be in a separate PR for the sake of git history.

Perhaps a separate commit at minimum as part of this PR would be a good idea.

@milosgajdos
Copy link
Member Author

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

@squizzi
Copy link
Collaborator

squizzi commented Jun 29, 2023

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

@milosgajdos milosgajdos merged commit bac7f02 into distribution:main Jun 29, 2023
@milosgajdos milosgajdos deleted the enable-build-tags branch June 29, 2023 21:47
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