Skip to content

Conversation

@juliakreger
Copy link

No description provided.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2019
@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 31, 2019
@derekhiggins
Copy link

I don't see prepare-ipxe.sh, did you forget to add it?
I'm also wondering what was wrong with the downstream only version (changed here #9), I didn't think openshift ci would be looking at this .ocp file)

@juliakreger juliakreger force-pushed the make-ocp-dockerfile-smarter branch from fac97fc to e62b548 Compare August 2, 2019 11:36
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 2, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2019
@derekhiggins
Copy link

Does there need to be two commits here?

Copy link
Member

@dtantsur dtantsur left a comment

Choose a reason for hiding this comment

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

Shouldn't we do it upstream? One day CentOS 8 will become a thing..

@elfosardo
Copy link

@dtantsur we should, actually we should keep upstream and downstream as close as possible
until centos8 is not here though, it looks hard, unless we could use ubi8

@dtantsur
Copy link
Member

dtantsur commented Aug 2, 2019

If we can confirm this code works with RHEL 8, we know it will work for CentOS 8, no?

@derekhiggins
Copy link

Does there need to be two commits here?

Now there is 3 commits, can we squash them into one?

@juliakreger
Copy link
Author

@dtantsur only once the downstream image sync occurs, and once merged it will be an email tomorrow.

@juliakreger
Copy link
Author

@derekhiggins Can we just squash with merge buttin in the github ui?

@juliakreger
Copy link
Author

@elfosardo We are using ubi8. The downstream team has no idea why this is happening :\

@juliakreger
Copy link
Author

FYI, the OpenShift nightly build failed because of this, so it would be good to move this forward ASAP as they are wanting to re-trigger the nightly builds.

Copy link
Member

@dtantsur dtantsur left a comment

Choose a reason for hiding this comment

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

/lgtm

Please consider my nits when creating an upstream version (and let's make sure we have an upstream version when we prove this working).

RUN mkdir /tftpboot && \
cp /usr/share/ipxe/undionly.kpxe /tftpboot/ && \
cp /usr/share/ipxe/ipxe.efi /tftpboot/ipxe.efi
copy ./prepare-ipxe.sh /tmp
Copy link
Member

Choose a reason for hiding this comment

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

nit: please update it to COPY when implementing upstream

@@ -0,0 +1,12 @@
#!/bin/bash -x
mkdir /tftpboot
Copy link
Member

Choose a reason for hiding this comment

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

nit: mkdir -p

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2019
@derekhiggins
Copy link

@derekhiggins Can we just squash with merge buttin in the github ui?

yup, have never that before but works for me

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekhiggins, dtantsur, elfosardo, juliakreger

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 [derekhiggins,dtantsur,elfosardo,juliakreger]

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

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. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants