feat(pkg/objstore/s3): support for prefixes #1392
Conversation
|
Going to fix the docs related stuff... as soon as I figure out how that works |
|
Ready to go 🎉 |
fix(docs/storage): run make docs chore: move up prefix parsing into parseConfig() Signed-off-by: Jared Allard <[email protected]>
Signed-off-by: Jared Allard <[email protected]>
| @@ -92,6 +94,11 @@ func parseConfig(conf []byte) (Config, error) { | |||
| config.PartSize = defaultMinPartSize | |||
| } | |||
|
|
|||
| // here for testing | |||
| if config.Prefix != "" { | |||
| config.Prefix = strings.TrimSuffix(config.Prefix, DirDelim) + DirDelim | |||
There was a problem hiding this comment.
for the configuration parsing test
There was a problem hiding this comment.
Adding normal code just for tests doesn't sound like a good idea, consider removing this.
Co-Authored-By: Povilas Versockas <[email protected]> Signed-off-by: Jared Allard <[email protected]>
Signed-off-by: Jared Allard <[email protected]>
|
@edevouge feel free to nullify this PR with your work, I needed to implement this for my own use cases, and I would much rather you get credit than I. |
|
So, it looks good, but why are we limiting ourselves to S3 implementation? If we are getting into this, I think it might be a part of our interface instead. E.g Adding thanos/pkg/objstore/client/factory.go Line 32 in 2c5f2cd This will be then consistent across all object storages. |
@jaredallard : I appreciate your contribution, but it would be great the next time you decided to implement a design proposal to post a small comment on the issue thread to explain your intention. Because, I was also writing some code for this feature and it's not really efficient to do the job twice. I'll let you finish the job. |
Good point, I'll rework this. |
|
hi folks, is this waiting for the equivalent implementation on the other object stores for merging? |
|
This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. |
|
This looks like a good feature, do you think it will re-open? |
| @@ -87,6 +89,11 @@ func parseConfig(conf []byte) (Config, error) { | |||
| return Config{}, err | |||
| } | |||
|
|
|||
| // here for testing | |||
There was a problem hiding this comment.
This looks wrong as @povilasv mentioned in https://github.com/thanos-io/thanos/pull/1392/files#r312902398
| @@ -90,6 +90,7 @@ config: | |||
| insecure_skip_verify: false | |||
| trace: | |||
| enable: false | |||
| prefix: "" | |||
There was a problem hiding this comment.
I would be curious and actually I think it's must-have to actually make this change on bucket level, not each bucket client level.. WDYT? (:
|
Well it really depends on @jaredallard if he is willing to continue this change. I believe to merge it we need to do it per global object storage config, not just S3, so all providers can reuse this prefix logic (: We can also discuss if it should be Help wanted, we definitely would like to have this in! |
|
Hey, I'm likely to not drive this work to completion. I am no longer on the team that needed this change at my company (they have maintained a fork with this AFAIK). I may come and do it in my spare time, but I have a massive backlog of things to do outside of work and this is pretty low on that list for that reason 😓 Sorry everyone! |
|
This is totally ok (: No worries, and good luck! It's best if you are transparent about the priorities, so someone else can take your work. :+1 Thanks! so... let's keep it close and help wanted (: |
Changes
I added support for s3 prefixes based on the amazing work proposed by #1318
Verification
Tested configuration changes via unit test, tested integration wise by running it.