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

cleanup: a small Azure driver cleanup #4138

Merged
merged 1 commit into from Oct 31, 2023

Conversation

milosgajdos
Copy link
Member

@milosgajdos milosgajdos commented Oct 27, 2023

Just so we can make the code a bit more consistent with the rest of the storage drivers.

Most of the updates in this PR are stylistic so the code across the drivers is more consistent in style.

Additions:

  • compile time interface checks for driver and writer
  • made some error messages a bit shorter
  • there are a few TODO comments around retries, concurrency (in writer - we set similar parameter in S3 driver to 100) and some if condition checks

PTAL @corhere

Just so we can make the code a bit more consistent with the rest of the
storage drivers.

Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
@@ -483,6 +499,8 @@ func is404(err error) bool {
)
}

var _ storagedriver.FileWriter = &writer{}
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure we need this here as newWriter returns storagedriver.FileWriter. We do add this check in the gcs driver so we should maybe remove it from there.

Though we should probably prefer returning struct types and not interfaces 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically we don't need it, though it doesn't hurt to be explicit about intent.

Though we should probably prefer returning struct types and not interfaces 🤔

In general, yes. In this instance, we have to return the interface-typed value for the driver to satisfy the storagedriver.StorageDriver interface.

Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

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

If a super-rigid coding style is a desirable goal, it'd be best to automate that task away with a super-opinionated code formatter like gofumpt.

@milosgajdos milosgajdos marked this pull request as ready for review October 27, 2023 20:14
@@ -452,10 +468,9 @@ func (d *driver) listBlobs(ctx context.Context, virtPath string) ([]string, erro
}
for _, blob := range resp.Segment.BlobItems {
if blob.Name == nil {
return nil, fmt.Errorf("required blob property Name is missing while listing blobs under: %s", listPrefix)
return nil, fmt.Errorf("missing blob Name when listing prefix: %s", listPrefix)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the original message is clearer in this context.

@@ -34,11 +37,15 @@ type driver struct {
copyStatusPollDelay time.Duration
}

type baseEmbed struct{ base.Base }
type baseEmbed struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

love this fmt, but it could be done by golint?

Copy link
Collaborator

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@milosgajdos milosgajdos merged commit d153e1d into distribution:main Oct 31, 2023
15 checks passed
@milosgajdos milosgajdos deleted the azure-cleanup branch October 31, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants