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

objstore/s3: Add list_objects_version to S3 config #3312

Merged
merged 1 commit into from
Oct 26, 2020
Merged

objstore/s3: Add list_objects_version to S3 config #3312

merged 1 commit into from
Oct 26, 2020

Conversation

sevagh
Copy link
Contributor

@sevagh sevagh commented Oct 13, 2020

Signed-off-by: Sevag Hanssian sevagh@protonmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

I added an optional config "list_objects_version: string" to the S3 configuration.

Verification

I have a bucket in production which requires ListObjectsV1 to work (I need to deploy a custom binary with minio UseV1 set to true). After making this PR, I tested "thanos bucket tools web" with the new config option and it works.

@sevagh
Copy link
Contributor Author

sevagh commented Oct 13, 2020

Solves #3291

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contributions.

Let's wait for another approval and then we can merge it.

@sevagh
Copy link
Contributor Author

sevagh commented Oct 15, 2020

ci/circleci: test — Your tests failed on CircleCI

I can't see what failed here. Is it related to my code?

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks! One suggestion for flexibility and LGTM (:

docs/storage.md Outdated Show resolved Hide resolved
@sevagh sevagh changed the title objstore/s3: Add list_objects_v1 to S3 config objstore/s3: Add list_objects_version to S3 config Oct 20, 2020
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Small suggestion and LGTM (:

pkg/objstore/s3/s3.go Show resolved Hide resolved
@sevagh
Copy link
Contributor Author

sevagh commented Oct 26, 2020

I rebased on the tip of master to fix the CHANGELOG conflict. This should be OK to merge if it passes the checks?

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing - rebase went wrong?

CHANGELOG.md Outdated

### Fixed
- [#3257](https://github.com/thanos-io/thanos/pull/3257) Ruler: Prevent Ruler from crashing when using default DNS to lookup hosts that results in "No such hosts" errors.
- [#3331](https://github.com/thanos-io/thanos/pull/3331) Disable Azure blob exception logging
- [#3341](https://github.com/thanos-io/thanos/pull/3341) Disable Azure blob syslog exception logging
- [#3276](https://github.com/thanos-io/thanos/pull/3276) Query Frontend: Support query splitting and retry for labels and series requests.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think rebase went wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CHANGELOG looks different from the time I had to modify it: 57076a5#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed

This has me confused, TBH.

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 just fixed it manually.

Signed-off-by: Sevag Hanssian <sevagh@protonmail.com>
@bwplotka
Copy link
Member

Thanks!

@bwplotka bwplotka merged commit 7fc7900 into thanos-io:master Oct 26, 2020
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.

3 participants