Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

feat: Initial Implementation of Storager #2

Merged
merged 6 commits into from
Sep 15, 2021

Conversation

abyss-w
Copy link
Collaborator

@abyss-w abyss-w commented Sep 3, 2021

feat: Initial Implementation of Storager

service.toml Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated Show resolved Hide resolved
storage.go Outdated
if err == nil {
// The directory exist, we should delete it.
// ref: https://github.com/beyondstorage/specs/blob/master/rfcs/134-write-behavior-consistency.md
_, err = s.client.NewDirectoryURL(path).Delete(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to delete the existing dir and recreate here. We can set the metadata for the existing dir object.

storage.go Outdated
return meta
}

func (s *Storage) nextObjectPageByDir(ctx context.Context, page *ObjectPage) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks the same with nextObjectPageByPrefix?

storage.go Outdated
return nil, err
}
// The directory exist, we should reset the metadata.
_, err = s.client.NewDirectoryURL(path).SetMetadata(ctx, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my inaccurate expression. We don't need to call SetMetadata here. We can use the metadata returned by GetProperties like lastModified to assign the value for o.lastModified. Like what we did in https://github.com/beyondstorage/go-service-kodo/blob/master/storage.go#L51-L60

o.SetContentType(v)
}
if v := fileOutput.ContentMD5(); len(v) > 0 {
o.SetContentMd5(base64.StdEncoding.EncodeToString(v))
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, our content md5 is based encoded, maybe we need to add description in content_md5.

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 7, 2021

This week is our 1st cleanup week, we will resume the work here in the next week: https://forum.beyondstorage.io/t/1st-cleanup-week-for-go-storage/213/12

BeyondStorage
Hello everyone, we will start a cleanup week from 9/6 to 9/10. In next week, we will stop any new feature development of go-storage and focus on bugfix, docs like: every project’s README.md update (update links of matrix room, integration tests, and so on) go-storage-example enhancement. go-storage user guide and developer guide. Clean up issues we are working on and plan to do. File new issues about any problems that meet (could be sovled later) … Next week, all our maintainers and committe...

}

// Since `Create' only initializes the file, we need to call `UploadRange' to write the contents to the file.
_, err = s.client.NewFileURL(path).UploadRange(ctx, 0, body, transactionalMD5)
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened if transactionalMD5 is empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is an optional value, so the null value does not affect the API call.

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 15, 2021

Most LGTM.

@Xuanwo Xuanwo merged commit c735d27 into beyondstorage:master Sep 15, 2021
@abyss-w abyss-w deleted the storager branch September 15, 2021 08:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants