Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AGENT-205: Fix OCP image build #3839

Merged
merged 1 commit into from
May 26, 2022

Conversation

vfreex
Copy link
Contributor

@vfreex vfreex commented May 23, 2022

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Assignees

/cc @
/cc @

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • Reviewers have been listed
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 23, 2022
@openshift-ci openshift-ci bot requested review from empovit and rewantsoni May 23, 2022 08:32
@openshift-ci openshift-ci bot added the kind/dependency-change Categorizes issue or PR as related to changing dependencies label May 23, 2022
@osherdp
Copy link
Contributor

osherdp commented May 23, 2022

I think usage of quay.io/ocpmetal/oc-image:bug-1823143-multi-arch-ai-bug-2069976 should be preserved, as the changes didn't get into oc 4.11
/hold
(just until we figure out the status of this patch, everything else looks good :) )

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 23, 2022
Copy link
Contributor

@eranco74 eranco74 left a comment

Choose a reason for hiding this comment

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

/hold
We need the custom oc for supporting installation with mirror registry

@lranjbar
Copy link
Contributor

/retest

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #3839 (76cdab7) into master (1611544) will increase coverage by 2.51%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3839      +/-   ##
==========================================
+ Coverage   66.33%   68.85%   +2.51%     
==========================================
  Files         175      178       +3     
  Lines       24629    27308    +2679     
==========================================
+ Hits        16338    18802    +2464     
- Misses       6777     6910     +133     
- Partials     1514     1596      +82     
Impacted Files Coverage Δ
internal/network/family_utils.go 50.00% <0.00%> (-12.50%) ⬇️
...nal/controller/controllers/bmh_agent_controller.go 62.38% <0.00%> (-0.45%) ⬇️
internal/ignition/ignition.go 57.50% <0.00%> (-0.33%) ⬇️
internal/common/db.go 0.00% <0.00%> (ø)
internal/host/conditions.go 100.00% <0.00%> (ø)
internal/provider/ovirt/inventory.go 0.00% <0.00%> (ø)
internal/common/test_configuration.go 0.00% <0.00%> (ø)
internal/provider/vsphere/inventory.go 0.00% <0.00%> (ø)
...rnal/controller/controllers/infraenv_controller.go 60.48% <0.00%> (ø)
internal/ignition/ironic.go 100.00% <0.00%> (ø)
... and 17 more

@lranjbar
Copy link
Contributor

/approve

Please note that this change is only to Dockerfile.assisted-service.ocp which is the assisted-service image landing in the OCP payload. From what I understand the Openshift policy is: "All the images in the OCP payload have to use the other official images in the OCP payload." We will have to get this fix in the official oc image in the OCP payload to support disconnected environments.

In the meantime, we need to have this image built by ART so that we can start testing the OCP payload official image for assisted-service properly. I will let others from the Openshift Agent Team weigh in on this but while this is unfortunate that this bug is still not fixed... this is likely the path forward for us.

@vfreex Thank you for opening this PR for us. 😃

Agent team: @dhellmann @zaneb @rwsu @pawanpinjarkar @andfasano @bfournie @celebdor

@openshift-ci
Copy link

openshift-ci bot commented May 23, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lranjbar, vfreex

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 May 23, 2022
@zaneb
Copy link
Member

zaneb commented May 23, 2022

If this just prevents us from doing disconnected installs then I think it is the right way to move forward for now. We may have to re-evaluate if the bug doesn't get a timely fix, but anything is better than nothing.
If the way that assisted-service interacts with the CLI does not work with the standard binary (e.g. passes a command-line argument that will be rejected?) then we might need to find another approach even in the short term. That's not going to involve using a base image from quay though, so I think we should go ahead with this change in either case.

/hold cancel

@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 May 23, 2022
@andfasano
Copy link
Contributor

/retest

2 similar comments
@lranjbar
Copy link
Contributor

/retest

@lranjbar
Copy link
Contributor

/retest

@empovit
Copy link
Contributor

empovit commented May 25, 2022

/uncc empovit

@openshift-ci openshift-ci bot removed the request for review from empovit May 25, 2022 16:40
@lranjbar
Copy link
Contributor

lranjbar commented May 25, 2022

JIRA issue tracker: AGENT-205

@lranjbar lranjbar changed the title Fix OCP image build AGENT-205: Fix OCP image build May 25, 2022
@pawanpinjarkar
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link

openshift-ci bot commented May 26, 2022

@vfreex: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 96765ee into openshift:master May 26, 2022
flaper87 pushed a commit to flaper87/assisted-service that referenced this pull request Sep 21, 2022
danielerez pushed a commit to danielerez/assisted-service that referenced this pull request Oct 15, 2023
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. kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants