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

Make it possible to supply a glob for matching files to remove from S3 #61

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sklirg
Copy link

@sklirg sklirg commented Feb 26, 2019

Hi there!

Today I tried out this plugin and I really like it.

I'm uploading files for a "static website" and they are generated with hashes in the filenames for cache busting. Rather than removing the hashes, I'd like to remove some files from the bucket before I upload new ones.

It's a long time since I wrote some Go code, so feel free to comment on it.

I'm also unfamiliar with any special conventions for Drone plugins, so I'm open for suggestions.

Best regards,
Sklirg

@techknowlogick
Copy link

Instead of regex, could standard file globing be used?

@sklirg
Copy link
Author

sklirg commented Feb 26, 2019

Zglob (which is used in this project already) assumes the files are on disk, but they are not, they're in S3.

I quickly looked up some libraries for globbing in golang, but being unfamiliar with them as well as not wanting to add further dependencies to the project I went with regex.

I'm open to switching to globs instead, if you have a library for this in mind. :)

@bradrydzewski
Copy link
Member

bradrydzewski commented Feb 26, 2019

the filepath package in the Go standard library supports globbing using filepath.Match. For consistency we should use globbing instead of regular expressions.

the only downside to the Go standard library is that they do not support ** syntax. This is why we use zglob in this particular plugin. We also use bmatcuk/doublestar in many other Drone projects.

@sklirg
Copy link
Author

sklirg commented Feb 27, 2019

Alright, I'll have a look at using those libs instead. Thanks for the pointers!

I found the main (plugin) function to get quite big, should I split out this feature out into a new function to make it easier to test etc.?

@sklirg sklirg force-pushed the feat/delete-regex branch 2 times, most recently from 9408d3b to 5e50f33 Compare February 27, 2019 17:49
@sklirg sklirg changed the title Make it possible to supply a regex for matching files to remove from S3 Make it possible to supply a glob for matching files to remove from S3 Mar 4, 2019
@sklirg
Copy link
Author

sklirg commented Mar 4, 2019

I rewrote it to use the globbing provided by doublestar now. Feel free to give any further comments. :)

plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated Show resolved Hide resolved
@sklirg
Copy link
Author

sklirg commented Mar 7, 2019

I've renamed the variables :)

Copy link
Contributor

@tboerger tboerger left a comment

Choose a reason for hiding this comment

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

Only some small nitpicking, beside that it looks pretty good to me.

plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated
// want to remove files from S3.
if !p.DryRun {
log.WithFields(log.Fields{
"num_files": len(removeIdentifiers),
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's fine to just call it files

plugin.go Outdated Show resolved Hide resolved
plugin.go Outdated
_, err := client.DeleteObjects(deleteInput)

if err != nil {
log.WithFields(log.Fields{
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 add log in err != nil condition since main.go can handle this.

Copy link
Author

Choose a reason for hiding this comment

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

Hm, the file currently does this for other code, i.e. here:

drone-s3/plugin.go

Lines 110 to 113 in 758ad2d

if err != nil {
log.WithFields(log.Fields{
"error": err,
}).Error("Could not match files")

I'm fine with removing my additions which do this, though. The only concern is that maybe the error messages from S3 are more obscure than the ones provided by the logs.

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.

5 participants