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.

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

Extend with plugins #2733

Merged
merged 4 commits into from
Apr 21, 2020
Merged

Extend with plugins #2733

merged 4 commits into from
Apr 21, 2020

Conversation

bigkevmcd
Copy link
Contributor

What type of PR is this?

/kind feature

What does does this PR do / why we need it:

It allows for external binaries to be used as plugins.

for example:

$ odo promote --from a --to

This would look for an executable called odo-promote on the path, and execute that, passing the parameters through.

How to test changes / Special notes to the reviewer:

Write the following to an executable script on your PATH called "odo-testing" (perhaps in ~/bin if you have that on your PATH).

#!/bin/sh
MESSAGE=$1
shift
ARGS=$@
echo "this is executing this command ${MESSAGE} with args ${ARGS}"

This should trigger executing your shell script

$ odo testing mytest and more`
this is executing this command mytest with args with more

@codecov
Copy link

codecov bot commented Mar 20, 2020 •
edited
Loading

Codecov Report

Merging #2733 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2733   +/-   ##
=======================================
  Coverage   43.20%   43.20%           
=======================================
  Files          97       97           
  Lines        8888     8888           
=======================================
  Hits         3840     3840           
  Misses       4684     4684           
  Partials      364      364

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78a1312...cbcb0d8. Read the comment docs.

@amitkrout
Copy link
Contributor

@bigkevmcd Thanks for the pr. I really don't understand what is the usecase and the acceptance criteria of this pr. May be you can add a document for it with more details.

Can you please create an issue and link to this pr. I am saying this because i feel we need to discuss properly and validate the thoughts behind it among the team. ping @girishramnani @kadel

@kadel
Copy link
Member

kadel commented Mar 24, 2020

@bigkevmcd Thanks for the pr. I really don't understand what is the usecase and the acceptance criteria of this pr. May be you can add a document for it with more details.

Can you please create an issue and link to this pr. I am saying this because i feel we need to discuss properly and validate the thoughts behind it among the team. ping @girishramnani @kadel

I had a brief discussion about this with @bigkevmcd on Slack.
It is probably the simplest plugin mechanism. It will allow others to prototype new odo commands.

I would treat is as a hidden easter egg that should be used only for demos and experiments.

Before making this available for anyone to use, I would like to have some kind of plugin management and distribution system (maybe something like https://krew.sigs.k8s.io/)

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Mar 24, 2020
@sbose78
Copy link
Member

sbose78 commented Mar 24, 2020

+1

@amitkrout
Copy link
Contributor

@bigkevmcd UT is failing at line - https://travis-ci.com/github/openshift/odo/jobs/300352024#L155. PTAL
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Mar 28, 2020
@kadel
Copy link
Member

kadel commented Apr 6, 2020

ping @bigkevmcd

@bigkevmcd
Copy link
Contributor Author

Sorry, I'll get to this tomorrow, been a bit hectic with other things, but we still would like to get this landed.

@kadel
Copy link
Member

kadel commented Apr 7, 2020

/hold cancel

@kadel kadel requested review from mik-dass and removed request for rajivnathan and maysunfaisal April 7, 2020 13:37
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 7, 2020
@kadel kadel requested review from rajivnathan, maysunfaisal and cdrage and removed request for rajivnathan and maysunfaisal April 7, 2020 13:38
girishramnani
girishramnani reviewed Apr 8, 2020
View reviewed changes
pkg/odo/cli/plugins/plugin_handler_test.go Outdated
if runtime.GOOS == "windows" {
t.Skip()
}
tempDir, cleanup := makeTempDir(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally unit tests shouldn’t interact with external entities such as a filesystem which are subject to change. Can you please use the mocked filesystem we have inplace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, @girishramnani Can you please add some reference where we have already done it. Reference would really be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@bigkevmcd bigkevmcd Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This looks like I'd need to reimplement https://golang.org/pkg/os/exec/#LookPath on top of afero.

In other words, replace https://github.com/golang/go/blob/master/src/os/exec/lp_unix.go#L34 with a version that worked on top of afero, purely for test purposes.

I'm not sure of the value of this? It wouldn't be testing the core logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe consider writing an integration test?

@girishramnani
Copy link
Contributor

/hold

Waiting for this to get resolved #2733 (comment)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 8, 2020
@mnuttall mnuttall mentioned this pull request Apr 8, 2020
Closed
@bigkevmcd
Copy link
Contributor Author

/test v4.3-integration-e2e-benchmark

@bigkevmcd bigkevmcd closed this Apr 9, 2020
@bigkevmcd bigkevmcd reopened this Apr 10, 2020
@bigkevmcd
Add support for external process plugins.
f241f53 
This adds support for external tools as plugins.

If the provided command is a known command, then it's executed,
otherwise, odo attempts to find a matching executable on the path with
the prefix "odo-" and execute that instead.

e.g. "odo tkn list" would attempt to execute "odo-tkn" and pass the args
on.
@sspeiche
Copy link
Contributor

Does this work take the place of #1173 or it is separate?

@sspeiche
Copy link
Contributor

See also #1173

@kadel
Copy link
Member

kadel commented Apr 17, 2020

Does this work take the place of #1173 or it is separate?

It os part of that.
I look at this as a way to validate this extension mechanism.
For now, gitops experiment should be the only one who uses that to validate that this is a useful way to do odo plugins. Once validated we should create some sort of plugin manager (something like Krew), before we announce it for wider adoption.

@kadel
Copy link
Member

kadel commented Apr 17, 2020 •
edited
Loading

@girishramnani can you please re-check if your concerns were addressed?

@girishramnani
Copy link
Contributor

/lgtm. 👍 @bigkevmcd

@girishramnani
Copy link
Contributor

/retest

@girishramnani
Copy link
Contributor

/hold cancel

@girishramnani
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 20, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 20, 2020
@openshift-bot
Copy link

/retest

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

4 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 0a68b6a into redhat-developer:master Apr 21, 2020
@cdrage cdrage added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girishramnani girishramnani girishramnani left review comments

@amitkrout amitkrout amitkrout left review comments

@mik-dass mik-dass Awaiting requested review from mik-dass

@cdrage cdrage Awaiting requested review from cdrage

Assignees

@girishramnani girishramnani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@bigkevmcd @amitkrout @kadel @openshift-ci-robot @sbose78 @girishramnani @sspeiche @openshift-bot @cdrage @openshift-merge-robot