Skip to content

Conversation

omer-vishlitzky
Copy link

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

openshift-ci-robot commented Jul 9, 2025

@omer-vishlitzky: This pull request references MGMT-20830 which is a valid jira issue.

In response to this:

https://issues.redhat.com/browse/MGMT-20830

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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2025
@openshift-ci openshift-ci bot requested review from eliorerz and ori-amizur July 9, 2025 13:07
Copy link

openshift-ci bot commented Jul 9, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: omer-vishlitzky
Once this PR has been reviewed and has the lgtm label, please assign avishayt for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link

codecov bot commented Jul 9, 2025

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.40%. Comparing base (81f7727) to head (65069e2).

Files with missing lines Patch % Lines
pkg/imagestore/imagestore.go 75.00% 1 Missing and 1 partial ⚠️
internal/handlers/long.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #449      +/-   ##
==========================================
+ Coverage   59.32%   59.40%   +0.07%     
==========================================
  Files          27       27              
  Lines        1571     1579       +8     
==========================================
+ Hits          932      938       +6     
- Misses        478      479       +1     
- Partials      161      162       +1     
Files with missing lines Coverage Δ
internal/handlers/short.go 86.36% <100.00%> (+0.64%) ⬆️
internal/handlers/long.go 90.00% <0.00%> (ø)
pkg/imagestore/imagestore.go 72.26% <75.00%> (-0.15%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

Do we actually have any of these (disconnected-interactive isos) yet to test with?

@@ -51,6 +51,8 @@ func parseShortURL(r *http.Request) (*imageDownloadParams, int, error) {
params.imageType = "minimal-iso"
case "full.iso":
params.imageType = "full-iso"
case "disconnected-interactive.iso":
params.imageType = "disconnected-interactive-iso"
Copy link
Member

Choose a reason for hiding this comment

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

This is the best name we have for this? It's fine if there are no other options, but it feels a bit unwieldy to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, naming is hard:) Maybe simply 'disconnected.iso'? @andfasano wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I feel that the interactive part plays quite a relevant role, since it's a special ISO meant not only to support the disconnected installation (ie like the regular ABI ISO), but also to spin up a local agent TUI and moreover a complete assisted UI to support the interactive installation process by the user. Anyhow I don't want to block on this, if no other better ideas will come up I'm fine with disconnected.iso

Copy link
Member

Choose a reason for hiding this comment

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

I see we're using OVE everywhere, is that not a good name for this? I guess it can be used for more than just installing virt and that's why we're avoiding it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, OVE (virt) is just the current use-case. But a similar ISO could be used also for other scenarios which require different operators (ACM/etc). I.e. OVE is a specific flow which requires virt operators, but the ISO would support (later on) selecting other operators depending on the use-case.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, OVE is only the current scope, so it will be better to avoid using it in the naming for a more generic product

Copy link
Member

Choose a reason for hiding this comment

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

In the context of assisted-service (where users will encounter this), all installs are interactive

  • disconnected-iso
  • self-contained-iso
  • jumbo-iso

not gonna use the 'a' word 🙊

Copy link
Member

Choose a reason for hiding this comment

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

I like just removing interactive.

+1 for disconnected-iso/disconnected.iso from me

@danielerez
Copy link
Contributor

/cc @andfasano

Copy link

openshift-ci bot commented Aug 21, 2025

@omer-vishlitzky: all tests passed!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants