Skip to content

Dedupe on-prem bootstrap manifests #2071

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Oct 29, 2020
Merged

Dedupe on-prem bootstrap manifests #2071

openshift-merge-robot merged 1 commit into from
Oct 29, 2020

Conversation

@bcrochet
Copy link
Member

@bcrochet bcrochet commented Sep 10, 2020

- What I did
Currently, all of the on-prem platforms duplicate the manifests needed for
bootstrap. There is a small difference in all of them, and these can be
gotten rid of by moving some of the template code into functions in Go.

- How to verify it
Verify that on-prem platforms can still deploy.

- Description for the changelog

Dedupe bootstrap manifests for on-prem platforms.

@mandre
Copy link
Member

mandre commented Sep 10, 2020

/test e2e-openstack
/test e2e-ovirt
/test e2e-vsphere

@ashcrow ashcrow requested review from sinnykumari and removed request for ashcrow September 10, 2020 16:49
@cgwalters cgwalters added the 4.7 Work deferred for 4.7 label Sep 10, 2020
cgwalters
cgwalters approved these changes Sep 10, 2020
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

/approve
the idea in general (and we should likely do the same with the kubelet systemd unit).

But this is a nontrivial change without an immediate bug it's fixing (right?) so I'm adding the 4.7 label.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2020
@bcrochet
Copy link
Member Author

bcrochet commented Sep 10, 2020

/approve
the idea in general (and we should likely do the same with the kubelet systemd unit).

But this is a nontrivial change without an immediate bug it's fixing (right?) so I'm adding the 4.7 label.

You are correct. Thanks!

@bcrochet
Copy link
Member Author

bcrochet commented Sep 10, 2020

/test e2e-metal-ipi
/test e2e-ovirt
/test e2e-openstack
/test e2e-vsphere
/test e2e-aws

@bcrochet
Copy link
Member Author

bcrochet commented Sep 11, 2020

/retest

cybertron
cybertron suggested changes Sep 11, 2020
View reviewed changes
Copy link
Member

@cybertron cybertron left a comment

Choose a reason for hiding this comment

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

Cool, this is a big improvement! A couple of things inline.

@bcrochet bcrochet force-pushed the manifest-dedupe branch 2 times, most recently from 7851af9 to 702e637 Compare September 11, 2020 22:16
@bcrochet
Copy link
Member Author

bcrochet commented Sep 14, 2020

/retest

@ravidbro
Copy link

ravidbro commented Sep 14, 2020

I'm thrilled to see that.
Are we planning to do the same also for the templates?

@bcrochet
Copy link
Member Author

bcrochet commented Sep 14, 2020

I'm thrilled to see that.
Are we planning to do the same also for the templates?

See #2079

cybertron
cybertron approved these changes Sep 14, 2020
View reviewed changes
Copy link
Member

@cybertron cybertron left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 14, 2020

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 14, 2020

/retest

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

@cybertron
Copy link
Member

cybertron commented Sep 14, 2020

/lgtm cancel

I don't think we want the bot to sit here and retest until 4.7 opens.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020
@bcrochet
Copy link
Member Author

bcrochet commented Sep 17, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2020
@bcrochet
Copy link
Member Author

bcrochet commented Oct 12, 2020

/retest

@bcrochet
Copy link
Member Author

bcrochet commented Oct 12, 2020

/test e2e-ovirt
/test e2e-openstack
/test e2e-vsphere

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 12, 2020

@bcrochet: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-upgrade 702e63716ca74d0e575668877acb5222df799f38 link /test e2e-gcp-upgrade
ci/prow/e2e-aws-workers-rhel7 4771fc3705391fdab825d034674d54441f185453 link /test e2e-aws-workers-rhel7
ci/prow/e2e-openstack 4771fc3705391fdab825d034674d54441f185453 link /test e2e-openstack
ci/prow/e2e-ovirt 4771fc3705391fdab825d034674d54441f185453 link /test e2e-ovirt
ci/prow/e2e-aws 4771fc3705391fdab825d034674d54441f185453 link /test e2e-aws
ci/prow/e2e-ovn-step-registry 4771fc3705391fdab825d034674d54441f185453 link /test e2e-ovn-step-registry

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

@bcrochet
Copy link
Member Author

bcrochet commented Oct 15, 2020

/retest

@bcrochet
Dedupe on-prem bootstrap manifests
eb9ee78 
Currently, all of the on-prem platforms duplicate the manifests needed for
bootstrap. There is a small difference in all of them, and these can be
gotten rid of by moving some of the template code into functions in Go.
@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test e2e-ovirt

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test e2e-gcp-op

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test e2e-agnostic-upgrade

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test okd-e2e-aws

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test e2e-aws

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test okd-e2e-aws

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test e2e-gcp-op

@ravidbro
Copy link

ravidbro commented Oct 27, 2020

/test okd-e2e-aws

@bcrochet
Copy link
Member Author

bcrochet commented Oct 28, 2020

/retest

@ravidbro
Copy link

ravidbro commented Oct 29, 2020

/test e2e-gcp-op

@ravidbro
Copy link

ravidbro commented Oct 29, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 29, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bcrochet, cgwalters, ravidbro

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:

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

@ravidbro ravidbro removed their assignment Oct 29, 2020
@openshift-merge-robot openshift-merge-robot merged commit 2299f82 into openshift:master Oct 29, 2020
@cgwalters
Copy link
Member

cgwalters commented Oct 29, 2020

Thanks for pushing this through!

@ravidbro ravidbro mentioned this pull request Nov 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgwalters cgwalters cgwalters approved these changes

@ericavonb ericavonb Awaiting requested review from ericavonb

@sinnykumari sinnykumari Awaiting requested review from sinnykumari

+1 more reviewer

@cybertron cybertron cybertron approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cybertron cybertron

Labels

4.7 Work deferred for 4.7 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bcrochet @mandre @ravidbro @openshift-bot @cybertron @openshift-ci-robot @cgwalters @openshift-merge-robot