Skip to content

Conversation

@tchap
Copy link
Contributor

@tchap tchap commented Jun 19, 2025

The change itself is fairly small, but there were quite a few tests using schema1. These had to be updated. Some test cases also don't make sense any more as you cannot really update an image schema2, that would change the hash. So tests updating an image with a signature were removed as signatures are not supported in schema2 implicitly.

Feel free to propose more test cases if anything comes up your mind, I did what I could without learning all the intricacies of the codebase, which meant extending the tests for imageutil.InternalImageWithMetadata for all schema types, then aligning all other breaking tests.

The PR is a bit chaotic in the sense that there are multiple mock manifests and configs scattered across the codebase. I normalized what made sense to me, but some chaos is still there. Particularly there are separate tests for filling in the image object from the string manifest and config, then there are API tests for actually posting a manifest. These are entirely separate tests.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 19, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 19, 2025

@tchap: This pull request references WRKLDS-1599 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set.

In response to this:

This is needed so that we can upgrade to distribution/[email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from adambkaplan and bparees June 19, 2025 08:44
@tchap tchap changed the title WRKLDS-1599: Drop support for image manifest schema 1 WIP: WRKLDS-1599: Drop support for image manifest schema 1 Jun 19, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2025
@tchap
Copy link
Contributor Author

tchap commented Jun 23, 2025

/retest

@tchap tchap force-pushed the drop-schema1 branch 2 times, most recently from 6e96234 to 58b821a Compare June 23, 2025 20:01
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 23, 2025

@tchap: This pull request references WRKLDS-1599 which is a valid jira issue.

In response to this:

This is needed so that we can upgrade to distribution/[email protected]

The change itself is fairly small, but there were quite a few tests using schema1. These must have been updated. Some test cases also don't make sense any more as you cannot really update an image schema2, that would change the hash. So tests updating an image with a signature were removed as signatures are not supported in schema2 implicitly.

Feel free to propose more test cases if anything comes up your mind, I did what I could without learning all the intricacies of the codebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 23, 2025

@tchap: This pull request references WRKLDS-1599 which is a valid jira issue.

In response to this:

This is needed so that we can upgrade to distribution/[email protected]

The change itself is fairly small, but there were quite a few tests using schema1. These must have been updated. Some test cases also don't make sense any more as you cannot really update an image schema2, that would change the hash. So tests updating an image with a signature were removed as signatures are not supported in schema2 implicitly.

Feel free to propose more test cases if anything comes up your mind, I did what I could without learning all the intricacies of the codebase, which meant extending the tests for imageutil.InternalImageWithMetadata for all schema types, then aligning all other breaking tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 23, 2025

@tchap: This pull request references WRKLDS-1599 which is a valid jira issue.

In response to this:

The change itself is fairly small, but there were quite a few tests using schema1. These must have been updated. Some test cases also don't make sense any more as you cannot really update an image schema2, that would change the hash. So tests updating an image with a signature were removed as signatures are not supported in schema2 implicitly.

Feel free to propose more test cases if anything comes up your mind, I did what I could without learning all the intricacies of the codebase, which meant extending the tests for imageutil.InternalImageWithMetadata for all schema types, then aligning all other breaking tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@tchap tchap changed the title WIP: WRKLDS-1599: Drop support for image manifest schema 1 WRKLDS-1599: Drop support for image manifest schema 1 Jun 23, 2025
@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 23, 2025
@tchap
Copy link
Contributor Author

tchap commented Jun 25, 2025

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 30, 2025

@tchap: This pull request references WRKLDS-1599 which is a valid jira issue.

In response to this:

The change itself is fairly small, but there were quite a few tests using schema1. These had to be updated. Some test cases also don't make sense any more as you cannot really update an image schema2, that would change the hash. So tests updating an image with a signature were removed as signatures are not supported in schema2 implicitly.

Feel free to propose more test cases if anything comes up your mind, I did what I could without learning all the intricacies of the codebase, which meant extending the tests for imageutil.InternalImageWithMetadata for all schema types, then aligning all other breaking tests.

The PR is a bit chaotic in the sense that there are multiple mock manifests and configs scattered across the codebase. I normalized what made sense to me, but some chaos is still there. Particularly there are separate tests for filling in the image object from the string manifest and config, then there are API tests for actually posting a manifest. These are entirely separate tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@sanchezl
Copy link
Contributor

sanchezl commented Jul 1, 2025

These types are duplicated in openshift/library-go pkg/image/dockerv1client/types.go

@tchap
Copy link
Contributor Author

tchap commented Jul 1, 2025

These types are duplicated in openshift/library-go pkg/image/dockerv1client/types.go

I guess I can just migrate to library-go in a subsequent PR and remove the packages as present in this repo.

@tchap
Copy link
Contributor Author

tchap commented Jul 2, 2025

/retest

1 similar comment
@tchap
Copy link
Contributor Author

tchap commented Jul 4, 2025

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 4, 2025

@tchap: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-cmd 99e8cd6 link false /test e2e-aws-ovn-cmd

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@sanchezl sanchezl left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanchezl, tchap

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 037953b into openshift:main Jul 7, 2025
9 of 10 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-openshift-apiserver
This PR has been included in build ose-openshift-apiserver-container-v4.20.0-202507071217.p0.g037953b.assembly.stream.el9.
All builds following this will include this PR.

@tchap tchap deleted the drop-schema1 branch July 11, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants