Skip to content

Conversation

@pablintino
Copy link
Contributor

@pablintino pablintino commented Dec 2, 2025

- What I did

Minor improvement to:

  • Ensure that pkg/controller/osimagestream/helpers_test.go testing data is created once per test case and not shared between test cases.
  • Make the GetOSImageStreamSetByName function return a k8s NotFound error in case the requested OSImageStreamSet does not exist.
  • Bugfix to ensure that URL based sources don't pass empty ImageDatas to the GroupOSContainerImageMetadataToStream function in case one or both images don't have enough labels to determine the stream. The same patch is applied to the ImageStream source.
  • Ensure that GroupOSContainerImageMetadataToStream never returns partial OSImageStreamSets with only one of the URLs filled.

- How to verify it

Unit tests are enough to test the functionality.

- Description for the changelog

Improved OSImageStreamSet retrieval error handling and fixed bugs with URL-based image source metadata grouping

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

openshift-ci-robot commented Dec 2, 2025

@pablintino: This pull request references MCO-1961 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.21.0" version, but no target version was set.

Details

In response to this:

- What I did

Minor improvement to:

  • Ensure that pkg/controller/osimagestream/helpers_test.go testing data is created once per test case and not shared between test cases.
  • Make the GetOSImageStreamSetByName function return a k8s NotFound error in case the requested OSImageStreamSet does not exist.

- How to verify it

Unit tests are enough to test the functionality.

- Description for the changelog

Refactored OS image stream test data isolation and improved GetOSImageStreamSetByName error handling to return a k8s NotFound error.

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2025
@pablintino
Copy link
Contributor Author

/verified by @pablintino

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Dec 2, 2025
@openshift-ci-robot
Copy link
Contributor

@pablintino: This PR has been marked as verified by @pablintino.

Details

In response to this:

/verified by @pablintino

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.

@pablintino pablintino force-pushed the osimagestream-followup-1 branch from f1d8e1d to 0d269f9 Compare December 3, 2025 10:27
@openshift-ci-robot openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Dec 3, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2025
@pablintino pablintino force-pushed the osimagestream-followup-1 branch from 0d269f9 to 4b24805 Compare December 3, 2025 10:31
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 3, 2025

@pablintino: This pull request references MCO-1961 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.21.0" version, but no target version was set.

Details

In response to this:

- What I did

Minor improvement to:

  • Ensure that pkg/controller/osimagestream/helpers_test.go testing data is created once per test case and not shared between test cases.
  • Make the GetOSImageStreamSetByName function return a k8s NotFound error in case the requested OSImageStreamSet does not exist.
  • Bugfix to ensure that URL based sources don't pass empty ImageDatas to the GroupOSContainerImageMetadataToStream function in case one or both images don't have enough labels to determine the stream. The same patch is applied to the ImageStream source.
  • Ensure that GroupOSContainerImageMetadataToStream never returns partial OSImageStreamSets with only one of the URLs filled.

- How to verify it

Unit tests are enough to test the functionality.

- Description for the changelog

Refactored OS image stream test data isolation and improved GetOSImageStreamSetByName error handling to return a k8s NotFound error.

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
Contributor

openshift-ci-robot commented Dec 3, 2025

@pablintino: This pull request references MCO-1961 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.21.0" version, but no target version was set.

Details

In response to this:

- What I did

Minor improvement to:

  • Ensure that pkg/controller/osimagestream/helpers_test.go testing data is created once per test case and not shared between test cases.
  • Make the GetOSImageStreamSetByName function return a k8s NotFound error in case the requested OSImageStreamSet does not exist.
  • Bugfix to ensure that URL based sources don't pass empty ImageDatas to the GroupOSContainerImageMetadataToStream function in case one or both images don't have enough labels to determine the stream. The same patch is applied to the ImageStream source.
  • Ensure that GroupOSContainerImageMetadataToStream never returns partial OSImageStreamSets with only one of the URLs filled.

- How to verify it

Unit tests are enough to test the functionality.

- Description for the changelog

Improved OSImageStreamSet retrieval error handling and fixed bugs with URL-based image source metadata grouping

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.

@pablintino pablintino force-pushed the osimagestream-followup-1 branch 2 times, most recently from d4f92d7 to 7cf1c7f Compare December 3, 2025 13:11
This change corrects a few things:
- Comments from the original content like ensuring helpers_test.go test
  data is created once per test case or making GetOSImageStreamSetByName
return a k8s NotFound error in case the requested stream doesn't exist.
- Bugfix to ensure that URL based sources don't pass empty ImageDatas to
  the GroupOSContainerImageMetadataToStream function in case one or both
images don't have enough labels to determine the stream. The same patch
is applied to the ImageStream source.
- Ensure that GroupOSContainerImageMetadataToStream never returns
  partial OSImageStreamSets with only one of the URLs filled.
@pablintino pablintino force-pushed the osimagestream-followup-1 branch from 7cf1c7f to 019e91e Compare December 3, 2025 13:14
@pablintino
Copy link
Contributor Author

/test e2e-aws-ovn

Comment on lines +126 to +129
// How can a stream end here?
// - An ImageStream with only one of the images tagged
// - A proper built ImageStream but one of the images is lacking the stream labels
// - A URL set with only one image having the labels
Copy link
Member

Choose a reason for hiding this comment

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

Nice comment here!

Copy link
Member

@isabella-janssen isabella-janssen left a comment

Choose a reason for hiding this comment

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

/lgtm

Changes look clear & updated units are still passing.

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

openshift-ci bot commented Dec 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: isabella-janssen, pablintino

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [isabella-janssen,pablintino]

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

@pablintino
Copy link
Contributor Author

/verified by @pablintino
The PR has UTs that cover the changed code. The entire functionality is not yet merged so no e2e available yet.

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Dec 3, 2025
@openshift-ci-robot
Copy link
Contributor

@pablintino: This PR has been marked as verified by @pablintino.

Details

In response to this:

/verified by @pablintino
The PR has UTs that cover the changed code. The entire functionality is not yet merged so no e2e available yet.

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.

@pablintino
Copy link
Contributor Author

/test e2e-gcp-op-2of2

@pablintino
Copy link
Contributor Author

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Dec 3, 2025
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 8e6beb0 and 2 for PR HEAD 019e91e in total

@yuqi-zhang
Copy link
Contributor

/override ci/prow/e2e-gcp-op-2of2
/override ci/prow/e2e-aws-ovn-upgrade

Should both be unrelated

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2025

@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/e2e-aws-ovn-upgrade, ci/prow/e2e-gcp-op-2of2

Details

In response to this:

/override ci/prow/e2e-gcp-op-2of2
/override ci/prow/e2e-aws-ovn-upgrade

Should both be unrelated

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 3, 2025

@pablintino: 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/bootstrap-unit 019e91e link false /test bootstrap-unit

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 5051cce into openshift:main Dec 3, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. 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. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants