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

use region specified in the BackupStorageLocation spec to detect the Bucket region #3617

Closed
wants to merge 1 commit into from
Closed

Conversation

jala-dx
Copy link
Contributor

@jala-dx jala-dx commented Mar 23, 2021

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#3616

Please indicate you've done the following:

Signed-off-by: Jalaja Ganapathy <jalaja@replicated.com>

Signed-off-by: Jalaja <jalaja@replicated.com>
Comment on lines +72 to +74
case location.Spec.Config["region"] != "":
region = location.Spec.Config["region"]
url = fmt.Sprintf("s3-%s.amazonaws.com", region)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified to keep all the region inferring/getting logic together by moving this into the default case.

default:
    region := location.Spec.Config["region"]
    var err error
    if region == "" {
        region, err = getASBucketRegion(bucket)
        if err != nil {
		url = "s3.amazonaws.com"
		break
	}
	url = fmt.Sprintf("s3-%s.amazonaws.com", region)
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of the switch statement altogether and just use an if/else clause. I don't think we want any additional cases here anyhow.

@@ -0,0 +1 @@
Region is calculated incorrectly when gov account is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use the PR title as the contents of the changelog?

Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

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

This LGTM.
I have a couple of comments.
Also, please add unit tests for this change.

@carlisia carlisia removed the request for review from dsu-igeek April 15, 2021 22:39
@zubron
Copy link
Contributor

zubron commented Apr 21, 2021

Hi @jala-dx! Are you able to make the requested changes from @ashish-amarnath? It would be great to get this PR merged :) Thanks!

Comment on lines +72 to +74
case location.Spec.Config["region"] != "":
region = location.Spec.Config["region"]
url = fmt.Sprintf("s3-%s.amazonaws.com", region)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of the switch statement altogether and just use an if/else clause. I don't think we want any additional cases here anyhow.

@zubron
Copy link
Contributor

zubron commented Jun 9, 2021

I tried to make some changes to this branch so we could get this merged but unfortunately maintainers don't have write permission for the branch. I've got another branch locally which includes 40a1b62, so I will close this PR and create a new one with the original commit from this PR and my follow up changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants