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

Plugin consolidate #6469

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

michaelgugino
Copy link
Contributor

@michaelgugino michaelgugino commented Dec 13, 2017

This commit relocates filter_plugings to lib_utils,
changes the namespacing to prevent unintended use of
older versions that may be present in filter_plugins/
directory on existing installs.

Add lib_utils to meta depends for roles

Also consolidate some plugins into lib_utils from
various other areas.

Update rpm spec, obsolete plugin rpms.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 13, 2017
@michaelgugino michaelgugino force-pushed the plugin-consolidate branch 2 times, most recently from 1871a49 to 5630457 Compare December 14, 2017 19:22
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017
@michaelgugino michaelgugino changed the title [WIP] Plugin consolidate Plugin consolidate Dec 14, 2017
@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 Dec 14, 2017
@mtnbikenc
Copy link
Member

All the symlinks for filter_plugins, lookup_plugins and library should be removed. You could also consolidate the callback from the installer_checkpoint role. The README.md would just need to be moved/renamed and the references to that role updated.

I don't know how much value adding lib_utils to the beginning of every filter give us.

Moving the openshift_quick_installer callback probably broke the quick installer.

@michaelgugino
Copy link
Contributor Author

I don't know how much value adding lib_utils to the beginning of every filter give us.

It gives us and other users an approximate idea of where this filter lives.

@michaelgugino
Copy link
Contributor Author

You could also consolidate the callback from the installer_checkpoint role. The README.md would just need to be moved/renamed and the references to that role updated.

I plan on more consolidation in a follow up.

@mtnbikenc mtnbikenc self-assigned this Dec 14, 2017
Copy link
Member

@mtnbikenc mtnbikenc left a comment

Choose a reason for hiding this comment

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

The changes look fine. I've run a 3.7 install and an upgrade to 3.8 successfully. I'm withholding the lgtm label because I'm not certain of the specfile changes or the impact to the quick installer.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2017
This commit relocates filter_plugings to lib_utils,
changes the namespacing to prevent unintended use of
older versions that may be present in filter_plugins/
directory on existing installs.

Add lib_utils to meta depends for roles

Also consolidate some plugins into lib_utils from
various other areas.

Update rpm spec, obsolete plugin rpms.
@sdodson
Copy link
Member

sdodson commented Dec 19, 2017

Specfile changes look good, I've verified the upgrade path works as expected from old to new packages.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@sdodson
Copy link
Member

sdodson commented Dec 19, 2017

I'll rebase #4264 to get rid of all the remaining subpackages once this merges.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 19, 2017

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

Test name Commit Details Rerun command
ci/openshift-jenkins/containerized 801779e link /test containerized
ci/openshift-jenkins/install 801779e link /test install
ci/openshift-jenkins/extended_conformance_install_crio 801779e link /test crio
ci/openshift-jenkins/system-containers 801779e link /test system-containers

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@sdodson
Copy link
Member

sdodson commented Dec 19, 2017

Install phase of those tests all completed. Looking to assess how soon the e2e tests will be fixed and may go ahead and merge without any e2e tests.

@sdodson sdodson merged commit 151115e into openshift:master Dec 19, 2017
mmckinst added a commit to mmckinst/openshift-ansible-contrib that referenced this pull request May 14, 2018
openshift/openshift-ansible#6469 relocated rpm_q.py to a
different directory.
cooktheryan pushed a commit to openshift/openshift-ansible-contrib that referenced this pull request Jun 4, 2018
openshift/openshift-ansible#6469 relocated rpm_q.py to a
different directory.
jaywryan pushed a commit to jaywryan/openshift-ansible-contrib that referenced this pull request Jul 3, 2018
openshift/openshift-ansible#6469 relocated rpm_q.py to a
different directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants