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

fix: use existing query params from blobstore response #136

Merged
merged 3 commits into from
Aug 10, 2024

Conversation

daniel-pebble
Copy link
Contributor

What this PR does / why we need it

This resolves an issue when pushing to a docker registry, it responses with a _state query param, this must be part of the put request. This PR, takes the query params from the response and appends the digest to the put request.

Which issue(s) this PR resolves / fixes

Resolves / Fixes #132

Please check the following list

  • Does the affected code have corresponding tests, e.g. unit test, E2E test? yes
  • Does this change require a documentation update? No
  • Does this introduce breaking changes that would require an announcement or bumping the major version? no
  • Do all new files have an appropriate license header? yes

@shizhMSFT shizhMSFT changed the title Use existing query params from blobstore response fix: use existing query params from blobstore response Aug 9, 2024
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.48%. Comparing base (c58adc9) to head (8317530).

Files Patch % Lines
src/OrasProject.Oras/Content/Digest.cs 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #136      +/-   ##
==========================================
+ Coverage   80.15%   80.48%   +0.33%     
==========================================
  Files          30       30              
  Lines         897      902       +5     
  Branches      108      109       +1     
==========================================
+ Hits          719      726       +7     
+ Misses        124      122       -2     
  Partials       54       54              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shizhMSFT
Copy link
Contributor

@daniel-pebble Thank you for your contribution. This PR contains the change of #135. Could you keep cc915c6 in this PR?

While testing against https://hub.docker.com/_/registry, "Docker-Content-Digest" headers were found on response.Headers collection rather than response.Content.Headers.
Aligned internal/private variables to use _camelCase as there was some inconsistencies.

Signed-off-by: Daniel Robinson <daniel.robinson@pebble.tv>
Use existing query parameter string from blobstore response and then append digest.  Test updated to ensure existing query parameters are maintained

Signed-off-by: Daniel Robinson <daniel.robinson@pebble.tv>
…56 and sha512 digest are registered the library can validate these although the library is limited to compute sha256 digest.

Signed-off-by: Daniel Robinson <daniel.robinson@pebble.tv>
@daniel-pebble
Copy link
Contributor Author

Hi @shizhMSFT, i've closed PR #135 and will commit under this PR to keep things simple. I've added some unit tests for the validate digest function. I'm not sure how I would go about adding unit tests for _camelCase naming convention, I've used sonarqube in the past to enforcing code styling, perhaps this is something Codecov can do? Happy to revert these changes if this requires a wider discussion?

@daniel-pebble
Copy link
Contributor Author

The other option of course is to use .editorconfig which would enforce _camalCase for private fields and PascalCase for public members.

I would suggest the following, Whats your thoughts on this?

Naming Styles

dotnet_naming_rule.private_fields_should_start_with_underscore.severity = error
dotnet_naming_rule.private_fields_should_start_with_underscore.symbols = private_fields
dotnet_naming_rule.private_fields_should_start_with_underscore.style = private_underscore_style

dotnet_naming_symbols.private_fields.applicable_kinds = field
dotnet_naming_symbols.private_fields.applicable_accessibilities = private
dotnet_naming_symbols.private_fields.required_modifiers =

dotnet_naming_style.private_underscore_style.capitalization = camel_case
dotnet_naming_style.private_underscore_style.required_prefix = _

dotnet_naming_rule.public_members_should_start_with_uppercase.severity = error
dotnet_naming_rule.public_members_should_start_with_uppercase.symbols = public_members
dotnet_naming_rule.public_members_should_start_with_uppercase.style = public_uppercase_style

dotnet_naming_symbols.public_members.applicable_kinds = property, field, method
dotnet_naming_symbols.public_members.applicable_accessibilities = public
dotnet_naming_symbols.public_members.required_modifiers =

dotnet_naming_style.public_uppercase_style.capitalization = pascal_case

@shizhMSFT
Copy link
Contributor

.editorconfig may work and worth a try.

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

src/OrasProject.Oras/Content/Digest.cs Show resolved Hide resolved
@shizhMSFT shizhMSFT merged commit 1f939a5 into oras-project:main Aug 10, 2024
7 checks passed
@daniel-pebble daniel-pebble deleted the AppendDigest branch August 10, 2024 08:23
@Wwwsylvia Wwwsylvia mentioned this pull request Sep 25, 2024
3 tasks
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.

Unable to push blobs to docker registry
2 participants