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

Feature/migrate to iam serviceaccount #42

Merged
merged 5 commits into from
Dec 6, 2022

Conversation

dtvalk-ov
Copy link
Contributor

Description

What

Please be specific and try to describe your thought process. State the obvious, since this might be the first time the reviewer is looking at the code

Why

https://financialtimes.atlassian.net/browse/UPPSF-3600

Anything, in particular, you'd like to highlight to reviewers

Mention here sections of code which you would like reviewers to pay extra attention to .E.g

Would appreciate a second pair of eyes on the test
I am not quite sure how this bit works
Is there a better library for doing x

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

This Pull Request follows the rules described in our Pull Requests Guide

@dtvalk-ov dtvalk-ov requested a review from a team as a code owner November 24, 2022 09:28
@dtvalk-ov dtvalk-ov requested review from a team and ManoelMilchev November 24, 2022 09:28
@coveralls
Copy link

coveralls commented Nov 29, 2022

Pull Request Test Coverage Report for Build 406

  • 13 of 22 (59.09%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 72.674%

Changes Missing Coverage Covered Lines Changed/Added Lines %
main.go 3 5 60.0%
s3.go 9 16 56.25%
Totals Coverage Status
Change from base Build 363: -0.7%
Covered Lines: 375
Relevant Lines: 516

💛 - Coveralls

@ivanruski ivanruski force-pushed the feature/migrate_to_iam_serviceaccount branch 3 times, most recently from 7ea4628 to ccdc578 Compare November 29, 2022 09:33
If deleting an object in s3 fail, it won't be reported in the error returned
from `svc.DeleteObjects`, instead it will be reported in the output object
returned from the same method.
@ivanruski ivanruski force-pushed the feature/migrate_to_iam_serviceaccount branch from ccdc578 to b61abd2 Compare November 29, 2022 09:40
@ivanruski ivanruski force-pushed the feature/migrate_to_iam_serviceaccount branch from b61abd2 to a3df679 Compare November 29, 2022 09:42
The service access buckets which are in eu-west-1, however when the service is
deployed in us the AWS_REGION envvars is set to us-east-1(because of the IAM to
ServiceAccount feature).
Copy link

@ManoelMilchev ManoelMilchev left a comment

Choose a reason for hiding this comment

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

LGTM!

merr = multierr.Append(merr, fmt.Errorf("deleting %q: %s", *e.Key, *e.Code))
}

return nil, err

Choose a reason for hiding this comment

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

question: Shouldn't this be merr?

Suggested change
return nil, err
return nil, merr

@dbelev dbelev merged commit 0153966 into master Dec 6, 2022
@dbelev dbelev deleted the feature/migrate_to_iam_serviceaccount branch December 6, 2022 10:59
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.

None yet

7 participants