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

Updated the KeyExists from aws to keep the response metadata in the image resolver #317

Merged

Conversation

mdupras
Copy link
Contributor

@mdupras mdupras commented Mar 15, 2023

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

When the image is resolved we validate if it exists with the method KeyExists. To validate this the methods fetch all the object metadata, if it fails it's considered not existent, but the result of that query isn't used.
This PR keeps the result and pass it to the AWS S3 image resolver.
I added an optional parameter for that metadata, in case the resolver needs to be build it elsewhere. If the metadata is not provided, it will fetch it like it used to.

Since I had overlapped change with #312, I included the change of those files in my PR to limit the conflicts and use the nullable options ;)

@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #317 (2acb001) into main (fb8b334) will decrease coverage by 1%.
The diff coverage is 64%.

@@         Coverage Diff         @@
##           main   #317   +/-   ##
===================================
- Coverage    85%    85%   -1%     
===================================
  Files        77     77           
  Lines      2195   2195           
  Branches    308    309    +1     
===================================
- Hits       1876   1873    -3     
- Misses      231    233    +2     
- Partials     88     89    +1     
Flag Coverage Δ
unittests 85% <64%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...oviders.AWS/Providers/AWSS3StorageImageProvider.cs 66% <63%> (-1%) ⬇️
...oviders.AWS/Resolvers/AWSS3StorageImageResolver.cs 92% <66%> (-8%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JimBobSquarePants
Copy link
Member

HI @mdupras thanks for this.

Would it be possible to revert the nullability changes since they overlap with the other PR which will actually increase the probability of conflicts since there have been changes requested there?

@mdupras mdupras force-pushed the mdupras/cached-metadata-query branch from 0eb19fc to 2acb001 Compare March 18, 2023 05:08
@mdupras
Copy link
Contributor Author

mdupras commented Mar 18, 2023

HI @mdupras thanks for this.

Would it be possible to revert the nullability changes since they overlap with the other PR which will actually increase the probability of conflicts since there have been changes requested there?

Yea no problem! Just did it!

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

Lovely stuff. Thanks!

@JimBobSquarePants JimBobSquarePants merged commit e13e8af into SixLabors:main Mar 18, 2023
@mdupras mdupras deleted the mdupras/cached-metadata-query branch October 19, 2023 12:50
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.

2 participants