Skip to content

Conversation

@kikisdeliveryservice
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice commented Jun 26, 2019

Remove all references to mcc/mcd/mcs/setupetdenv images so that we rely on the new MCO mega image only.

Note: Dockerfile.rhel7 images will be removed in a followup PR pending ART merge

Requires: openshift/installer#1875
Related-to: #739

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] *: remove refs to component images [WIP] : remove refs to component images Jun 26, 2019
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] : remove refs to component images [WIP] * : remove refs to component images Jun 26, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019
@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Jun 26, 2019

expected failure until i remove the component images from the installer (this is the first flag here bootkube.sh) via this PR openshift/installer#1875:
bootkube.sh[23882]: F0626 02:35:58.973555 1 main.go:29] Error executing mcc: unknown flag: --setup-etcd-env-image

kikisdeliveryservice added a commit to kikisdeliveryservice/machine-config-operator that referenced this pull request Jun 26, 2019
As an intermediate PR, as part of folding mco images into one image,
remove the required flag for mcc/mcd/mcs/setupetcdenv images in bootstrap.go

Required-by: openshift/installer#1875
Required-by: openshift#895
Related-to: openshift#739
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok gotcha. this one was trickier than the others and i wasn't 100% sure on it. thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think bc the other times i passed the MCO image with the command override I wasn't totally sure here about whether this is a delete or a replace with MCO image. Willl update!!

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 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 Jul 3, 2019
@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Jul 3, 2019

Cleaning this up to pull in the changes from #899 for testing

@runcom
Copy link
Member

runcom commented Jul 4, 2019

Can we also remove all the Dockerfile.* for the components not needed anymore and just live with a single, pure Dockerfile and Dockerfile.rhel7 (requires tweakings to hack/{build-image.sh,push-image.sh and Makefile)

@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 6, 2019
@kikisdeliveryservice
Copy link
Contributor Author

Can we also remove all the Dockerfile.* for the components not needed anymore and just live with a single, pure Dockerfile and Dockerfile.rhel7 (requires tweakings to hack/{build-image.sh,push-image.sh and Makefile)

Yep. In Progress.

@openshift-ci-robot openshift-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 8, 2019
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 8, 2019
@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Jul 9, 2019

Need to rename Dockerfile refs in release so that these updates using the simplified name Dockerfile work. (See PR: openshift/release#4314)

Have tested both scripts locally. Still need to update Makefile.

@kikisdeliveryservice
Copy link
Contributor Author

kikisdeliveryservice commented Jul 9, 2019

In order to rename our new master dockerfile which is currently called Dockerfile.machine-config-operator to just Dockerfile but not break CI, I think I need to:

  • in a seperate PR add a dockerfile named Dockerfile to the MCO repo (this is just a copy of the current Dockerfile.machine-config-operator - our dockerfile that makes all of our images)
  • merge PR ci-operator/mco: rename mco dockerfile release#4314
  • update ART (pr open waiting for approval)
  • delete the old Dockerfile.machine-config-operator (the old master dockerfile containing all the component images) in another PR or just via this PR

@kikisdeliveryservice
Copy link
Contributor Author

/retest

@kikisdeliveryservice
Copy link
Contributor Author

Since this can't be merged until ART hack/makefile changes are separate PRs. Will rebase this onto #950 and #948 which has those changes.

That said, @runcom couldn't I make the non-Rhel 7 changes from this PR and then land the Rhel7 changes in another PR after we get that sorted? The release PR already merged.

@runcom
Copy link
Member

runcom commented Jul 11, 2019

That said, @runcom couldn't I make the non-Rhel 7 changes from this PR and then land the Rhel7 changes in another PR after we get that sorted? The release PR already merged.

sure thing, hopefully that ART PR gets merged today also

Remove all references to mcc/mcd/mcs/setupetdenv images in
images.go, bootstrap.go and configmap.yaml

Related-to: openshift#739
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2019
Note: Dockerfile...rhel7 component images will be removed in a followup PR
pending ART merge

Related-to: openshift#739
kikisdeliveryservice added a commit to kikisdeliveryservice/machine-config-operator that referenced this pull request Jul 11, 2019
Since all component images have been folded into 1 master mco image,
remove rhel7 dockerfiles pending merge of ocp-build-data/merge_requests/181

Related-to: openshift#739
Followup-to: openshift#895
@kikisdeliveryservice kikisdeliveryservice changed the title [WIP] * : remove refs to component images * : remove refs to component images & dockerfiles Jul 12, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2019
@kikisdeliveryservice
Copy link
Contributor Author

@runcom split out rhel7 dockerfile deletions into #959 since ART is having some issues merging the PR. They are working on it. Took off the WIP on this on so it's ready (pending this passing e2e)

@runcom
Copy link
Member

runcom commented Jul 12, 2019

/retest
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kikisdeliveryservice, runcom

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 [kikisdeliveryservice,runcom]

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

@openshift-merge-robot openshift-merge-robot merged commit 43b202c into openshift:master Jul 12, 2019
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/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.

4 participants