fix(revert): reverts "fix implicit version handling of an artifact identity (#1026)" #1217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…omponent-model#1026)" This reverts commit 782970c.
github-actions
bot
added
kind/bugfix
frewilhelm
reviewed
Jan 2, 2025
Comment on lines
11
to
12
| // Replace enables to replace existing elements (same raw identity) with a different version instead | ||
| // of appending a new element. |
There was a problem hiding this comment.
This comment could stay, i guess
There was a problem hiding this comment.
Should I touch the original revert to accomodate this?
There was a problem hiding this comment.
yes, the comment sounds valid and independent from the changes. However, I would add the comment on an additional commit (not rebased or --amend)
There was a problem hiding this comment.
it will be squashed either way so that is a bit moot. I can add it into the final PR on merge.
There was a problem hiding this comment.
i forgot about the squash :O
I thought it would be beneficial to have the revert as separate commit in case we need the changes in the future. However, this option is probably not valid and, thus, irrelevant.
| @@ -33,7 +33,7 @@ func (o *Option) AddFlags(fs *pflag.FlagSet) { | |||
| fs.BoolVarP(&o.Actual, "actual", "", false, "use actual component descriptor") | |||
| fs.BoolVarP(&o.Update, "update", "U", false, "update digests in component version") | |||
| fs.BoolVarP(&o.Verify, "verify", "V", false, "verify digests found in component version") | |||
| fs.StringVarP(&o.outfile, "outfile", "O", "-", "Output file for normalized component descriptor") | |||
| fs.StringVarP(&o.outfile, "outfile", "O", "norm.ncd", "Output file for normalized component descriptor") | |||
There was a problem hiding this comment.
do you know what the reason for the change to - was?
There was a problem hiding this comment.
This was because the output file was written by default. I can patch this back in if you want
There was a problem hiding this comment.
Probably also a reasonable change that should stay and is independent from the revert.
Do we have an issue for that? |
|
Does this PR close #1214? |
|
yes, and this is a revert specifically to target 1214 and other side effects discovered during debugging (described in the PR title) |
Yes otherwise the diff checker would fail |
github-actions
bot
added
area/documentation
I edited the PR description to link the issue |
jakobmoellerdev
force-pushed
the
fix/revert-signing-version-identity-default
branch
from
a13c82b to
93d9f28
Compare
|
superceded by a better normalization algorithm |
jakobmoellerdev
added a commit
that referenced
this pull request
Jan 8, 2025
…ion/v1 as well as jsonNormalisation/v2 (#1218) <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Package jsonv3 provides a normalization which is completely based on the abstract (internal) version of the component descriptor and is therefore agnostic of the final serialization format. Signatures using this algorithm can be transferred among different schema versions, as long as is able to handle the complete information using for the normalization. jsonv2 is the predecessor of this version but had internal defaulting logic that is no longer included as part of this normalization. Thus v3 should be preferred over v2. Note that between v2 and v3 differences can occur mainly if the "extra identity" field is not unique, in which case the v2 normalization opinionated on how to differentiate these items. This no longer happens in v3, meaning the component descriptor is normalized as is. v2 and v1 were adjusted to accomodate the old(but new because forgotten) legacy behavior in legacy.go. Without this, old signatures would not work. This means this should be (at least partially) back-ported to the last minor versioned released without this correction. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> This issue fixes #1214 and supercedes #1217 as a better solution longterm (by getting rid of the old normalization) and shortterm (by achieving full backwards compatibility + introducing a simple test case) Note that this changes the default normalization algorithm to be `jsonNormalisation/v3` instead of `jsonNormalisation/v1` as it is important for users to migrate as soon as possible.
jakobmoellerdev
added a commit
to jakobmoellerdev/ocm
that referenced
this pull request
Jan 8, 2025
…ion/v1 as well as jsonNormalisation/v2 (open-component-model#1218) <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Package jsonv3 provides a normalization which is completely based on the abstract (internal) version of the component descriptor and is therefore agnostic of the final serialization format. Signatures using this algorithm can be transferred among different schema versions, as long as is able to handle the complete information using for the normalization. jsonv2 is the predecessor of this version but had internal defaulting logic that is no longer included as part of this normalization. Thus v3 should be preferred over v2. Note that between v2 and v3 differences can occur mainly if the "extra identity" field is not unique, in which case the v2 normalization opinionated on how to differentiate these items. This no longer happens in v3, meaning the component descriptor is normalized as is. v2 and v1 were adjusted to accomodate the old(but new because forgotten) legacy behavior in legacy.go. Without this, old signatures would not work. This means this should be (at least partially) back-ported to the last minor versioned released without this correction. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> This issue fixes open-component-model#1214 and supercedes open-component-model#1217 as a better solution longterm (by getting rid of the old normalization) and shortterm (by achieving full backwards compatibility + introducing a simple test case) Note that this changes the default normalization algorithm to be `jsonNormalisation/v3` instead of `jsonNormalisation/v1` as it is important for users to migrate as soon as possible.
ikhandamirov
pushed a commit
that referenced
this pull request
Jan 8, 2025
…ion/v1 as well as jsonNormalisation/v2 (#1218) (#1230) Note that this introduces a json normalization algorithm v3 which technically will break a minor release. however this is needed as we currently broke hashing and need to offer a working alternative. thus it should have been part of v0.19.0 already: #1214 <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Package jsonv3 provides a normalization which is completely based on the abstract (internal) version of the component descriptor and is therefore agnostic of the final serialization format. Signatures using this algorithm can be transferred among different schema versions, as long as is able to handle the complete information using for the normalization. jsonv2 is the predecessor of this version but had internal defaulting logic that is no longer included as part of this normalization. Thus v3 should be preferred over v2. Note that between v2 and v3 differences can occur mainly if the "extra identity" field is not unique, in which case the v2 normalization opinionated on how to differentiate these items. This no longer happens in v3, meaning the component descriptor is normalized as is. v2 and v1 were adjusted to accomodate the old(but new because forgotten) legacy behavior in legacy.go. Without this, old signatures would not work. This means this should be (at least partially) back-ported to the last minor versioned released without this correction. #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. --> This issue fixes #1214 and supercedes #1217 as a better solution longterm (by getting rid of the old normalization) and shortterm (by achieving full backwards compatibility + introducing a simple test case) Note that this changes the default normalization algorithm to be `jsonNormalisation/v3` instead of `jsonNormalisation/v1` as it is important for users to migrate as soon as possible. This is a backport of a93ecca in #1218
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 782970c.
What this PR does / why we need it
We have multiple regressions that were introduced with this change that did not get noticed properly.
considering that a simple
ocm get cvused to return:and now returns
even though the repository is unchanged is already problematic because the defaulting logic now applies not only with a transfer but also if it is not persisted in the repository. this causes side effects because now the signatures will change even through normalization because the new default behavior is differing.
There is also a bug in #1214 that describes an issue where after this change the signing hash calculation changes (because the defaulting also affects the component descriptor parsing involuntarily) which is unacceptable, because existing component version hashes are not allowed to change. This needs to be tested with a follow up PR.
There is furthermore an issue in the debuggability of the commandline and this change as it is impossible to have been tested with a case where extraidentity was set for one version but not for the other. before this PR is reintroduced this should be addressed.
Which issue(s) this PR fixes
Fixes #1214