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.

Sign up for GitHub

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

Jump to bottom

Catalog: adjust checksums-related constants and logic to adhere to the spec #3887

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Catalog: adjust checksums-related constants and logic to adhere to the spec #3887

merged 2 commits into from
Feb 23, 2024

Conversation

nl0
Copy link
Member

@nl0 nl0 commented Feb 21, 2024 •
edited
Loading

Description

  • Consistently refer to the new checksums as "modern"
  • Adhere to the spec (use sha2-256-chunked type/name and always double-hash)

TODO

  • Unit tests
  • Confirm that this change meets security best practices and does not violate the security model
  • ?? Documentation
  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

@codecov Codecov
Copy link

codecov bot commented Feb 21, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 36.24%. Comparing base (47055f7) to head (6901bc2).

Files Patch % Lines
.../containers/Bucket/PackageDialog/PackageDialog.tsx 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3887   +/-   ##
=======================================
  Coverage   36.24%   36.24%           
=======================================
  Files         716      716           
  Lines       31627    31627           
  Branches     4688     4689    +1     
=======================================
  Hits        11462    11462           
  Misses      19009    19009           
  Partials     1156     1156
Flag Coverage Δ
api-python 91.53% <ø> (ø)
catalog 10.62% <90.00%> (ø)
lambda 87.52% <ø> (ø)

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

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

@sir-sigurd
Copy link
Member

scope is unclear

@nl0 nl0 changed the title "modern" checksums Catalog: "modern" checksums Feb 21, 2024
@dimaryaz
Copy link
Contributor

I don't know if using the word "modern" is a good idea, especially in user-visible places like docs. What happens if we change the format again - will we call it "post-modern"?

@nl0
Copy link
Member Author

nl0 commented Feb 21, 2024

I don't know if using the word "modern" is a good idea, especially in user-visible places like docs.

well, yeah, maybe. any ideas? "multipart" seems somewhat imprecise, since it only refers to one particular case

will we call it "post-modern"?

i like that 😄

@nl0
Copy link
Member Author

nl0 commented Feb 21, 2024

smth like "adaptive" maybe? or just "multi" referring to multiformats

@fiskus
Copy link
Member

fiskus commented Feb 21, 2024

Maybe, if it's default format, and legacy has its legacy prefix, then no additional name is good? Just checksums

@nl0 nl0 changed the title Catalog: "modern" checksums Catalog: "modern" (chunked) checksums Feb 22, 2024
@nl0 nl0 force-pushed the adjust-checksum-type branch from 2fdaf37 to ca087ee Compare February 22, 2024 16:51
dimaryaz
dimaryaz previously approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@dimaryaz dimaryaz left a comment

Choose a reason for hiding this comment

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

Looks good - just remember to undo the CI changes

sir-sigurd
sir-sigurd previously approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@sir-sigurd sir-sigurd left a comment

Choose a reason for hiding this comment

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

I didn't dig into tests, otherwise LGTM.

@nl0 nl0 force-pushed the adjust-checksum-type branch from 9814e7b to 6901bc2 Compare February 23, 2024 10:07
@nl0 nl0 requested a review from sir-sigurd February 23, 2024 10:07
@nl0 nl0 changed the title Catalog: "modern" (chunked) checksums Catalog: chunked checksums (adjust constants and logic to adhere to the spec) Feb 23, 2024
@nl0 nl0 changed the title Catalog: chunked checksums (adjust constants and logic to adhere to the spec) Catalog: adjust checksums-related constants and logic to adhere to the spec Feb 23, 2024
@nl0 nl0 added this pull request to the merge queue Feb 23, 2024
Merged via the queue into master with commit 84867b0 Feb 23, 2024
40 checks passed
@nl0 nl0 deleted the adjust-checksum-type branch February 23, 2024 10:25
nl0 added a commit that referenced this pull request Feb 23, 2024
@nl0
Merge branch 'master' into lambdas-modern-checksums
cf160ff 
* master:
  Speedup promote in pkgpush (#3884)
  Catalog: adjust checksums-related constants and logic to adhere to the spec (#3887)
  shared: Tweak checksum types (#3888)
  Add new pkgpush types to quilt-shared (#3896)
  Add copy_file_list_fn parameter for Package._build() (#3895)
  Bump cryptography from 42.0.2 to 42.0.4 in /lambdas/indexer (#3891)
  pkgpush: adjust max_pool_connections (#3890)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sir-sigurd sir-sigurd sir-sigurd approved these changes

@dimaryaz dimaryaz dimaryaz left review comments

@fiskus fiskus Awaiting requested review from fiskus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nl0 @sir-sigurd @dimaryaz @fiskus