Skip to content

RFC-0005: introduction of Digest and change of Revision format#1001

Merged
hiddeco merged 17 commits intomainfrom
artifact-digest
Feb 14, 2023
Merged

RFC-0005: introduction of Digest and change of Revision format#1001
hiddeco merged 17 commits intomainfrom
artifact-digest

Conversation

@hiddeco
Copy link
Copy Markdown
Member

@hiddeco hiddeco commented Jan 18, 2023 •
edited
Loading

This PR introduces the changes described in RFC-0005, effectively deprecating the Checksum field of the Artifact in favor of the newly introduced Digest. While also changing the format of the Revision field for values containing a named-pointer and/or checksum.

  • API: Introduce Digest field in Artifact
  • API: Introduce TransformLegacyRevision to help (consumers) transitioning to the new revision format
  • Calculate Digest for Artifact when stored in Storage
  • Include Digest from Artifact in event metadata
  • Change Revision format for GitRepository
  • Change Revision format for Bucket
  • Change Revision format for OCIRepository
  • Change Revision format for HelmRepository
  • Allow configuration of digest algorithm using flag
  • Document specs

@hiddeco hiddeco added enhancement New feature or request area/storage Storage related issues and pull requests labels Jan 18, 2023
@hiddeco hiddeco force-pushed the artifact-digest branch 3 times, most recently from 583a3ff to bdd0717 Compare January 18, 2023 12:00
@stefanprodan stefanprodan mentioned this pull request Feb 7, 2023
Closed
21 tasks
@hiddeco hiddeco force-pushed the artifact-digest branch 4 times, most recently from 9a6d422 to 460ed45 Compare February 8, 2023 13:50
souleb
souleb reviewed Feb 8, 2023
View reviewed changes
Comment thread internal/helm/repository/chart_repository.go
@hiddeco hiddeco force-pushed the artifact-digest branch 5 times, most recently from 4e43e5e to 8252ce1 Compare February 9, 2023 16:54
@hiddeco hiddeco marked this pull request as ready for review February 9, 2023 17:00
stefanprodan
stefanprodan reviewed Feb 10, 2023
View reviewed changes
Comment thread api/v1beta2/artifact_types.go
Comment thread controllers/helmrepository_controller.go
darkowlzz
darkowlzz reviewed Feb 10, 2023
View reviewed changes
Comment thread api/v1beta2/artifact_types.go Outdated
darkowlzz
darkowlzz reviewed Feb 10, 2023
View reviewed changes
Comment thread api/v1beta2/artifact_types.go Outdated
darkowlzz
darkowlzz reviewed Feb 10, 2023
View reviewed changes
Comment thread api/v1beta2/artifact_types.go Outdated
@hiddeco
api: introduce Digest field to Artifact
964b2d3 
As discussed in RFC-0005, this introduces a `Digest` field to the
`Artifact` in favor of the now deprecated `Checksum`.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco requested a review from darkowlzz February 10, 2023 19:31
@hiddeco hiddeco mentioned this pull request Feb 13, 2023
Merged
stefanprodan
stefanprodan approved these changes Feb 13, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @hiddeco 🏅

@hiddeco hiddeco mentioned this pull request Feb 13, 2023
Merged
darkowlzz
darkowlzz reviewed Feb 13, 2023
View reviewed changes
Comment thread internal/digest/digest_test.go
Comment thread controllers/gitrepository_controller.go
darkowlzz
darkowlzz reviewed Feb 13, 2023
View reviewed changes
Comment thread controllers/ocirepository_controller.go Outdated
hiddeco added 16 commits February 14, 2023 12:48
@hiddeco
storage: calculate Digest for Artifact
6e0a6f1 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
reconcilers: include artifact digest in event meta
a72badf 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
digest: register SHA1 digest algorithm
f4eae19 
This algorithm is used by Git commit SHAs, and opens up the digest API
to work with these references.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: use digest for Bucket revision
83b6fdc 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: use RFC-0005 format for Git revision
fe8bc43 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: make HelmChart compat with RFC-0005
b89b049 
This ensures the revision is correctly parsed for `Bucket` and
`GitRepository` sources from which a chart is built, either in the
legacy or new RFC-0005 format.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
helm: use digest lib for checksum calculation
909ece4 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: make OCIRepository compat with RFC-0005
469c938 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
api: introduce TransformLegacyRevision helper
eaa4a4f 
This allows consumers to better handle the transition to the new
RFC-0005 format ("/" -> "@" separation).

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: use TransformLegacyRevision helper
f00aeae 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
controllers: RFC-0005 fmt for HelmRepository rev
0aaeeee 
This includes changes to the `ChartRepository`, to allow calculating
the revision and digest and tidy things.

In addition, the responsibility of caching the `IndexFile` has been
moved to the reconcilers. As this allowed to remove a lot of
complexities within the `ChartRepository`, and prevented passing on
the cache in general.

Change `HelmRepository`'s Revision to digest

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Allow config using --artifact-digest-algo
ccad359 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
spec: show RFC-0005 format and digest in examples
eb4a38e 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
misc: order imports and align digest aliases
d62f4dc 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Use Artifact.Path for HelmRepository index cache
f53bfd1 
Resolving it to a local path does not make it more unique, while
resulting in longer keys and a lot of safejoin calls.

Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco
Use MetaDigestKey from event API
9283894 
Signed-off-by: Hidde Beydals <hello@hidde.co>
@hiddeco hiddeco mentioned this pull request Feb 14, 2023
Closed
3 tasks
darkowlzz
darkowlzz approved these changes Feb 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

@hiddeco hiddeco merged commit e24ce86 into main Feb 14, 2023
@hiddeco hiddeco deleted the artifact-digest branch February 14, 2023 13:42
This was referenced Feb 14, 2023
Merged
Closed
@hiddeco hiddeco mentioned this pull request Feb 22, 2023
Merged
hiddeco added a commit that referenced this pull request Feb 22, 2023
@hiddeco
helm: only use Digest to calculcate index revision
c0a1099 
In #1001 bits around the Helm repository reconciliation logic were
rewritten, mostly based on the documented behavior instead of the
actual code. This resulted in the reintroduction of a YAML marshal of
the (sorted) index YAML instead of reliance of just the checksum of the
file.

This to take situations into account in which a repository would e.g.
provide a new random order on every generation. However, this approach
is (extremely) expensive as the marshal goes through a JSON -> YAML
loop, eating lots of RAM in the process.

As the further (silently) introduced behavior has not resulted in any
reported issues, I deem this approach safe and better than e.g.
encoding to just JSON which would still require a substantial amount of
memory.

Signed-off-by: Hidde Beydals <hello@hidde.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@souleb souleb souleb left review comments

@stefanprodan stefanprodan stefanprodan approved these changes

+1 more reviewer

@darkowlzz darkowlzz darkowlzz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/storage Storage related issues and pull requests enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hiddeco @darkowlzz @stefanprodan @souleb