drone-plugins / drone-s3-cache

Caches build artifacts to S3 compatible storage backends
http://plugins.drone.io/drone-plugins/drone-s3-cache
Apache License 2.0
28 stars 30 forks source link

Fix path joining #27

Closed donny-dont closed 6 years ago

donny-dont commented 6 years ago

Looks like the paths got screwed up after @tonglil 's commit. This uses path to join the filename to the paths

donny-dont commented 6 years ago

Thanks @jmccann it appears that @tboerger needs to add us both to the approved reviewers for this repo though 😢

tonglil commented 6 years ago

Sorry about that my bad.

donny-dont commented 6 years ago

@tonglil no worries. Found it relatively quick. One thing I'm not sure of though now that I'm looking a bit more at whats going on. Should it always append the root path? Currently its only doing it when it generates the path.

tonglil commented 6 years ago

The root is prefixed onto the paths if it's specified (for example setting the base bucket).

donny-dont commented 6 years ago

Sorry I guess I wasn't clear. Would users expect root to be appended if path is not length 0?

    // Get the path to place the cache files
    path := c.GlobalString("path")

    // Defaults to <owner>/<repo>/<branch>/
    if len(path) == 0 {
        log.Info("No path specified. Creating default")

        path = fmt.Sprintf(
            "%s/%s/%s",
            c.String("repo.owner"),
            c.String("repo.name"),
            c.String("commit.branch"),
        )

        path = prefixRoot(root, path)
    }
tonglil commented 6 years ago

If they specify a path, then the final location is: path If they specify a root, then the final location is: root + default path If they specify nothing, then the final location is: default path

Does that make sense/am I interpreting your question correctly?

EDIT: ah I see, path = prefixRoot(root, path) should be outside of if len(path) == 0 { if they specify a root and a path for the final location: root + path

tonglil commented 6 years ago

@donny-dont I think we should leave it as is; the root prefix only applies to default path, fallbackPath, and flushPath parameters. One can specify those parameters to override the root.

What do you think?

donny-dont commented 6 years ago

Its fine as long as its documented why you would do it. Guessing primarily for setting the bucket manually.