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

ceph: pass region to newS3agent() #8766

Merged
merged 1 commit into from Sep 23, 2021

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Sep 21, 2021

Description of your changes:
If the region is specified in the storage class of OBC, use that in the
newS3agent() than using constant "us-east-1".

Signed-off-by: Jiffin Tony Thottan thottanjiffin@gmail.com

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: Add the flag for skipping the build if this is only a documentation change. See here for the flag.
  • Skip Unrelated Tests: Add a flag to run tests for a specific storage provider. See test options.
  • Reviewed the developer guide on Submitting a Pull Request
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
  • Pending release notes updated with breaking and/or notable changes, if necessary.
  • Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
  • Code generation (make codegen) has been run to update object specifications, if necessary.

If the region is specified in the storage class of OBC, use that in the
newS3agent() than using constant "us-east-1".

Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz requested a review from leseb September 21, 2021 07:41
@mergify mergify bot added the ceph main ceph tag label Sep 21, 2021
func newS3Agent(accessKey, secretKey, endpoint, region string, debug bool, tlsCert []byte, insecure bool) (*S3Agent, error) {
var cephRegion = "us-east-1"
if region != "" {
cephRegion = region
Copy link
Member

Choose a reason for hiding this comment

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

How about a simple unit test that checks the region was applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was not able to find a way to mock s3 client for this unit test

Copy link
Member

Choose a reason for hiding this comment

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

Can't we read the region out of sess?

Copy link
Member

Choose a reason for hiding this comment

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

Another approach could be a helper method to get the region, which would be unit tested. While this small of an issue isn't critical for unit testing, in general we really want to have the mindset of unit testing everything within reason. For now though let's get it into the release today.

@travisn travisn merged commit 09a5b08 into rook:master Sep 23, 2021
@thotz thotz deleted the regionfixobcprovisioning branch September 23, 2021 17:37
mergify bot added a commit that referenced this pull request Sep 23, 2021
ceph: pass region to newS3agent() (backport #8766)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ceph main ceph tag
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants