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

Removing collections package and API cleanup #1501

Merged
merged 11 commits into from
Apr 15, 2021
Merged

Conversation

aloknerurkar
Copy link
Contributor

@aloknerurkar aloknerurkar commented Mar 26, 2021

@aloknerurkar aloknerurkar requested a review from acud March 26, 2021 11:30
@aloknerurkar aloknerurkar linked an issue Apr 1, 2021 that may be closed by this pull request
@aloknerurkar aloknerurkar changed the title WIP: Removing collections package and API cleanup Removing collections package and API cleanup Apr 9, 2021
@aloknerurkar aloknerurkar self-assigned this Apr 9, 2021
@aloknerurkar aloknerurkar marked this pull request as draft April 9, 2021 14:09
@aloknerurkar aloknerurkar marked this pull request as ready for review April 9, 2021 14:09
Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

great work! can we eliminate dirs endpoint?

pkg/api/bzz.go Outdated Show resolved Hide resolved
pkg/api/bzz.go Outdated Show resolved Hide resolved
pkg/api/bzz_test.go Show resolved Hide resolved
pkg/api/bzz_test.go Outdated Show resolved Hide resolved
pkg/api/dirs.go Outdated Show resolved Hide resolved
pkg/api/pin_files_test.go Outdated Show resolved Hide resolved
pkg/api/tag_test.go Show resolved Hide resolved
pkg/manifest/mantaray.go Outdated Show resolved Hide resolved
@aloknerurkar aloknerurkar requested a review from zelig April 12, 2021 07:38
@aloknerurkar
Copy link
Contributor Author

great work! can we eliminate dirs endpoint?

Yes we can technically remove it now itself. It was listed as a separate work item, so was not sure if there is anything else we need to do.

Beekeeper tests need to change. Will be looking into that now.

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

This is really great stuff :) I have a few minor comments, but otherwise this looks great! 🙏

openapi/Swarm.yaml Outdated Show resolved Hide resolved
pkg/api/bzz.go Outdated Show resolved Hide resolved
pkg/api/bzz.go Outdated Show resolved Hide resolved
pkg/api/bzz.go Show resolved Hide resolved
pkg/api/bzz.go Outdated Show resolved Hide resolved
pkg/api/bzz.go Show resolved Hide resolved
pkg/api/bzz_test.go Show resolved Hide resolved
http.StatusBadRequest,
jsonhttptest.WithRequestBody(bytes.NewReader(simpleData)),
jsonhttptest.WithExpectedJSONResponse(jsonhttp.StatusResponse{
Message: "invalid content-type header",
Copy link
Member

Choose a reason for hiding this comment

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

the pattern we usually use is to define the error as a package variable (unexported), then export it using export_test, then check for the exact error which is exported in the `export_test_ file in the tests, instead of checking for the hardcoded text value (this allows to change the error messages later on quite easily)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went down a rabbit hole here. There are too many. I tried to do everything in the bzz/dirs files.

pkg/api/dirs_test.go Outdated Show resolved Hide resolved
jsonhttptest.WithExpectedResponse([]byte(expectedResponse)),
)
}

validateBzzPath := func(t *testing.T, fromPath, toPath string) {
validateAltPath := func(t *testing.T, fromPath, toPath string) {
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 the rename? I think valdiateBzzPath captures it no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function validates that the file served on both the 'fromPath' and 'toPath' is the same (for redirects).

I named it 'AltPath' for 'alternate path'. I think this is better no?

@aloknerurkar aloknerurkar linked an issue Apr 12, 2021 that may be closed by this pull request
@aloknerurkar aloknerurkar requested a review from acud April 12, 2021 21:02
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Looks good 👍 minor non-blockers otherwise good to go.
Merge checklist:

  • beekeeper adjusted tests pass a cluster running this branch
  • beekeeper PR merged and new version tagged
  • this PR can be merged then

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.

Deprecate /dirs endpoint Eliminate collections package, remove /files endpoint
3 participants