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

Refactor package structure #86

Merged
merged 16 commits into from
Apr 5, 2020
Merged

Conversation

kakkoyun
Copy link
Contributor

@kakkoyun kakkoyun commented Nov 9, 2019

This PR changes the package structure to make it more maintainable. This need has risen after several backends merged.

Fixes several issues.
Improves automated tests.
Adopts stream-based IO processing.

Proposed Changes

Checklist

  • Read the CONTRIBUTING document.
  • Read the CODE OF CONDUCT document.
  • Add tests to cover changes.
  • Ensure your code follows the code style of this project.
  • Ensure CI and all other PR checks are green OR
    • Code compiles correctly.
    • Created tests which fail without the change (if possible).
    • All new and existing tests passed.
  • Add your changes to Unreleased section of CHANGELOG.
  • Improve and update the README (if necessary).
  • Ensure documentation is up-to-date. The same file will be updated in plugin index when your PR is accepted, so it will be available for end-users at http://plugins.drone.io.

@kakkoyun kakkoyun added the WIP Work in progress label Nov 9, 2019
@kakkoyun kakkoyun changed the title Refactor archive behaviour to log compressin ratio Refactor archive behaviour to log compression ratio Nov 9, 2019
@kakkoyun kakkoyun changed the title Refactor archive behaviour to log compression ratio Log actual compression ratio Nov 9, 2019
@kakkoyun kakkoyun added do not merge enhancement New feature or request labels Feb 7, 2020
@kakkoyun kakkoyun changed the title Log actual compression ratio Refactor package structure Feb 7, 2020
@kakkoyun
Copy link
Contributor Author

kakkoyun commented Mar 1, 2020

Update for the eyes of accidental reviewer:

This branch has gotten out of control. however please bear with me. It introduces a major overhaul on the structure and flow. This changes had been urged after the addition of several storage backends to the project. Roughly %70 of the work is done. It mostly misses tests, it'd be ready after necessary tests are added.

All the issue will that would be fixed when this got merged is listed in the description.

@aweris
Copy link

aweris commented Mar 9, 2020

I think this PR too big to merge. I think you should split this PR to a couple of smaller PR's

Copy link

@aweris aweris left a comment

Choose a reason for hiding this comment

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

Excellent work, well done!

Added a few recommendations about general commit structure and changesets.

Since it's a huge PR, I think you should replan this PR and split it smaller PR's and reorganize your commits. Each commit should explicit and contain a single change.

.golangci.yml Outdated Show resolved Hide resolved
Comment on lines +14 to +15
GORELEASER_BIN=$(GOPATH)/bin/goreleaser
LICHE_BIN=$(GOPATH)/bin/liche
Copy link

Choose a reason for hiding this comment

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

$(GOPATH)/bin used a couple of times already. Maybe it could be a good idea to move this to GOBIN=$(GOPATH)/bin

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 will address this in another PR

cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
.drone.yml Show resolved Hide resolved
cache/archive/option.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
cache/cache.go Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@kakkoyun
Copy link
Contributor Author

@aweris Thanks for the great review and feedback. After I'm done, I will separate it into more digestible chunks a thorough review.

@kakkoyun kakkoyun mentioned this pull request Mar 13, 2020
10 tasks
@kakkoyun kakkoyun removed WIP Work in progress do not merge labels Mar 15, 2020
@kakkoyun kakkoyun force-pushed the compression_rate branch 5 times, most recently from fe3870e to fa866ce Compare March 28, 2020 18:16
@kakkoyun kakkoyun marked this pull request as ready for review April 2, 2020 17:27
@kakkoyun
Copy link
Contributor Author

kakkoyun commented Apr 2, 2020

I have cleaned the commit history. This is now ready to merge.
Thanks, @aweris and silent others for the comments.

@kakkoyun kakkoyun force-pushed the compression_rate branch 7 times, most recently from 952c087 to 729d146 Compare April 5, 2020 14:27
@kakkoyun
Copy link
Contributor Author

kakkoyun commented Apr 5, 2020

I'm gonna go ahead and merge this. I'll address if someone reviews it (unlikely :)) and find an issue.

@kakkoyun kakkoyun merged commit 97fce2d into meltwater:master Apr 5, 2020
@kakkoyun kakkoyun deleted the compression_rate branch April 5, 2020 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants