Skip to content

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

Merged
JimBobSquarePants merged 2 commits into
SixLabors:mainfrom
mdupras:mdupras/cached-metadata-query
Mar 18, 2023
Merged

Updated the KeyExists from aws to keep the response metadata in the image resolver#317
JimBobSquarePants merged 2 commits into
SixLabors:mainfrom
mdupras:mdupras/cached-metadata-query

Conversation

@mdupras

@mdupras mdupras commented Mar 15, 2023

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Mar 15, 2023

Copy link
Copy Markdown

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
Copy Markdown
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

mdupras commented Mar 18, 2023

Copy link
Copy Markdown
Contributor Author

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!

@JimBobSquarePants JimBobSquarePants left a comment

Copy link
Copy Markdown
Member

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