Contribute svcat #1664
Contribute svcat #1664
Conversation
Do you want to try to keep all the history of development? I wouldn't want people to miss out on that. |
I don't have strong feelings on the git history. Honestly, I'm not even sure how hard/possible it is to move code across repos with different structures and keep the history... 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carolynvs maybe you could use the version that's injected via LDFLAGS at https://github.com/kubernetes-incubator/service-catalog/blob/master/Makefile#L74? I think that could be done in a follow-up (along with uploading the thing to GH releases, etc...)
Otherwise, this looks like a good foundation to me. I think we should follow-on with appropriate additional commands and contributing code/ideas back to SIG-CLI. I know @jberkhahn has been doing a lot of that already
I'm LGTM-ing this and am looking forward to hearing what others think.
@carolynvs looks like the CI failure is legit: https://travis-ci.org/kubernetes-incubator/service-catalog/builds/327880013#L574 |
Yes, how embarrassing! 😉 I found the makefile and will use that to get the code up to par. |
My only concern is I would like some more robust unit tests. What we have so far seems pretty spartan. |
Looks like it's @carolynvs @arschles & @sozercan who have all the history. If you're comfortable losing the history, that's fine with me. It's possible and not too hard to keep it if you're interested. |
Are there license issues that arise from retaining the git history?
…On Thu, Jan 11, 2018 at 7:27 PM, Morgan Bauer ***@***.***> wrote:
Looks like it's @carolynvs <https://github.com/carolynvs> @arschles
<https://github.com/arschles> & @sozercan <https://github.com/sozercan>
who have all the history. If you're comfortable losing the history, that's
fine with me. It's possible and not too hard to keep it if you're
interested.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1664 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAWXmAa07BvvCjE2S4aYPE3X8OYXDTAbks5tJqbdgaJpZM4RbeE2>
.
|
|
||
## Bash | ||
``` | ||
curl -sLO https://servicecatalogcli.blob.core.windows.net/cli/latest/$(uname -s)/$(uname -m)/svcat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the built versions continue to be built and hosted by this URI after the tool have been moved into service-catalog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned in the OP that these would need to be changed once we start building and publishing binaries during the build.
I can go into the how's here, but perhaps it's better left to a follow-on PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go ahead and land this so it doesn't get stale - some suggestions for follow-up below.
@@ -0,0 +1,211 @@ | |||
# Service Catalog CLI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider moving some of this to the doc dir in a follow-up.
"github.com/spf13/cobra" | ||
) | ||
|
||
type getCmd struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for a follow-up, just a note while I'm looking: I think that we should find another name for 'get' that disambiguates it from kubectl get
. I think this command adds a lot of value, but see it as distinct from what I would ordinarily think of as 'get'. What about something like browse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a deliberate choice to keep as closely as possible to kubectl's grammar, which drove the choice not only of the verbs get and describe, but also using the same "binary verb noun" pattern.
It makes for a much easier learning curve when people's default instincts honed from using kubectl just work with svcat. The output of svcat's get and describe were patterned after kubectl's output (though I didn't take it all the way due to time constraints).
I want to line-up svcat so that eventually when get and describe work well natively in kubectl, we can delegate those commands back to kubectl. Ideally resulting in as few changes as possible for the end user, so they can seamlessly switch from svcat get broker foo
to kubectl get broker foo
.
cmd/svcat/class/get_cmd.go
Outdated
Short: "List classes, optionally filtered by name", | ||
Example: ` | ||
svcat get classes | ||
svcat get class azure-mysqldb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a note for a follow-up: can we scrub azure
from the help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on that now! 👍 Switching everything over to the user provided service broker.
I am filling out the unit tests today. I had a branch where I was working on integration tests for svcat, but I left it out of this since a) it hadn't landed in svcat's master branch yet and b) it probably needs to change anyway to work with service catalog's integration test suite setup. I can do a follow-on issue/PR to hook up the integration tests. |
Honestly, I would be okay with the unit tests in a follow-up, assuming that everyone else is. I'll leave that to you all to sort out. In general I tend to have a bias toward 'merge now, fix things in follow-up' when it comes to code donations, but that's up to you all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that came to mind this morning is that I think the help text should document that this command should be considered alpha and subject to change. That might be helpful to add into this PR or definitely a narrowly scoped immediate follow-up PR.
} | ||
|
||
// NewDescribeCmd builds a "svcat describe class" command | ||
func NewDescribeCmd(cxt *command.Context) *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably another one that we should rename away (in a follow-up, not this PR) from the kubectl verb to something else, maybe inspect?
Let's do the CYA wording in the helptext on a follow-on PR. I'd like more time to come up with the proper wording for that. |
I've addressed the following items in this PR:
The remaining items will be addressed in follow-on PRs:
|
@carolynvs looks like this needs rebase now |
Yup! I am working this morning on rebasing my PRs after the 1.9 bump went in. |
Detect when running as a kubectl plugin and load flags from the kubectl environment variables
* github.com/spf13/viper * github.com/olekukonko/tablewriter * package: github.com/hashicorp/go-multierror
$ svcat install plugin --help Install as a kubectl plugin Usage: svcat install plugin [flags] Examples: svcat install plugin svcat install plugin --plugins-path /tmp/kube/plugins Flags: -p, --plugins-path string The installation path. Defaults to KUBECTL_PLUGINS_PATH, if defined, otherwise the plugins directory under the KUBECONFIG dir. In most cases, this is ~/.kube/plugins.
Also now that we have viper, --kubeconfig is set automatically to KUBECONFIG
* Integration tests were planned for commands that mutate state because the value of extensively mocking these commands is too low. * Switch tests to use ups broker data from the walkthrough.
Otherwise when you run tests they may fail depending on the local timezone. Not 100% sure if the user experience is better with local times or UTC, but that can be considered in a follow-up.
fc316ca
to
70a0801
Compare
Rebased and the checks are passing. |
I found an existing bug report for kubectl not supporting boolean flags (such as |
I'm going to go ahead and pull the trigger on this; looks like it's in good enough shape to pull into the repo, and we can mess with it all we want in follow-ups. |
This moves svcat out of Azure/service-catalog-cli and into this repo.
Package Structure
Commands
install plugin
- Copies the binary to the plugins directory, and generates a plugin manifest.get
anddescribe
for broker, class, plan, instance and bindingsync broker
provision
,deprovision
,bind
,unbind
Dev Tasks
go build ./cmd/svcat
compiles the binary.go test ./cmd/svcat/...
runs the unit tests. I've been working on integration tests but those aren't ready to contribute yet.TODO/Missing
I'm just calling out things that would probably need to happen, if this is accepted.
-X main.commit=$(COMMIT) -X main.version=$(VERSION)
where commit and version came from git.