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

[TT-1802] Clean up Stow locations when deleting middleware #22

Merged
merged 13 commits into from
Mar 17, 2021

Conversation

jlucktay
Copy link
Contributor

@jlucktay jlucktay commented Mar 16, 2021

Description

The handler to delete middleware was not cleaning up containers for either of the two different kinds of stow location in use by MServ i.e. local and s3.

api package

(api.API).HandleDeleteBundle(...)

  • added calls for the following:
  • refactored to get middleware from store at start of handler but not delete from store until the end, to reduce overall chance of failure leaving orphaned middlewares behind (de2b1eb)

General

  • exported the FmtPluginContainer format string used for container name layouts (94705cb, 81b5a70)
  • used exported stow.Kind consts in GetFileStore() rather than hard-coded strings (18cf3d3)

http_funcs package

  • added TestDeleteMW to cover the existing and updated functionality of the API's delete handler (1cb44d8)
  • broke tests and their helpers down across several files, instead of just one (cba2f6c)

conf package

  • refactored to add a Reload() func which will flush the current config and re-read data (c8e1a32)

util/conf package

  • added Flush() to facilitate conf.Reload()

util/storage/mock package

  • implemented some funcs necessary for increased test coverage

Miscellaneous

  • disabled maligned in the golangci-lint config file, as it has since moved into go vet (8cfe5ee)

Related Issue

TT-1802.

Motivation and Context

End users don't want S3 buckets left lying around after they delete a middleware.

Test Coverage For This Change

Added TestDeleteMW, and ran some integration tests on my local, to cover both local and s3 kinds of stow locations.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). If PRing from your fork, don't come from your master!
  • Make sure you are making a pull request against our master branch (left side). Also, it would be best if you started your change off our latest master.
  • My change requires a change to the documentation.
    • If you've changed APIs, describe what needs to be updated in the documentation.
  • I have updated the documentation accordingly.
  • Modules and vendor dependencies have been updated; run go mod tidy && go mod vendor in applicable directories.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Check your code additions will not fail linting checks:
    • gofmt -s -w .
    • go vet ./...
    • golangci-lint run

jlucktay added 13 commits March 15, 2021 10:06
… disk

Resetting the configuration for mserv to zero and (re)reading afresh aids testability.
Added a new test, along with some helpers and mock implementations, to cover the DeleteMW. Note that
the test is currently failing as of this commit, so let's see about fixing that!
There was previously no call made from the bundle delete handler for stow to remove its local/S3
container, so local files and S3 buckets would be left lying around after their corresponding
middleware definitions within MServ wer long gone.

fix TT-1802
The RemoveContainer implementation for "local" Kind in stow does not use the full path when
deleting, only a relative one. If WasabiAiR/stow#239 were to be
implemented/resolved then this block of code could be deleted.
…er, for S3's sake

Via Stow, S3 buckets can only be removed once they no longer contain any objects.

fix TT-1802
… beginning

We still do a GetMWByID call on the middleware in the store at the start of the handler, which would
error and return immediately if middleware with that ID did not exist. Given that succeeds though,
we proceed to clean out the stow container and remove that, which is much more error-prone given the
new changes in this feature branch. Assuming all of that succeeds, then we delete the middleware
from the store.
@jlucktay jlucktay requested a review from a team March 16, 2021 15:21
@jlucktay jlucktay marked this pull request as ready for review March 16, 2021 15:21
@jlucktay jlucktay merged commit e14ebfa into master Mar 17, 2021
@jlucktay jlucktay deleted the fix/TT-1802/clean-up-stow-locations branch March 17, 2021 09:45
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.

2 participants