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

Enhance code for issue 6297 #6326

Merged
merged 1 commit into from
May 31, 2023
Merged

Conversation

Lyndon-Li
Copy link
Contributor

Enhance the code because of #6297, the return value of GetBucketRegion is not recorded, as a result, when it fails, we have no way to get the cause.

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2023

Codecov Report

Merging #6326 (6fcc2c5) into main (4322ae1) will increase coverage by 1.96%.
The diff coverage is 0.00%.

❗ Current head 6fcc2c5 differs from pull request most recent head c3f7bb6. Consider uploading reports for the commit c3f7bb6 to get more accurate results

@@            Coverage Diff             @@
##             main    #6326      +/-   ##
==========================================
+ Coverage   40.97%   42.94%   +1.96%     
==========================================
  Files         255      222      -33     
  Lines       23833    22664    -1169     
==========================================
- Hits         9766     9733      -33     
+ Misses      13305    12168    -1137     
- Partials      762      763       +1     
Impacted Files Coverage Δ
pkg/repository/config/aws.go 20.00% <0.00%> (-2.73%) ⬇️

... and 34 files with indirect coverage changes

if requestErrs == nil {
return "", errors.New("unable to determine bucket's region")
} else {
return "", goerr.Join(requestErrs...)
Copy link
Contributor

Choose a reason for hiding this comment

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

would be helpful to make the error more friendly like appending the bucket to the error message,
for each request error, append the regionhint <- not sure if this is helpful...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@reasonerjt
Copy link
Contributor

Please update the title of the PR and commit since this will not fix the particular issue, but only helps debugging the problem.

danfengliu
danfengliu previously approved these changes May 31, 2023
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@Lyndon-Li Lyndon-Li changed the title Fix issue 6297 Enhance code for issue 6297 May 31, 2023
@Lyndon-Li Lyndon-Li merged commit d6f5e38 into vmware-tanzu:main May 31, 2023
22 checks passed
@Lyndon-Li Lyndon-Li deleted the issue-fix-6297 branch May 31, 2023 09:05
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