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

756-vgo-support #762

Closed
wants to merge 5 commits into from
Closed

756-vgo-support #762

wants to merge 5 commits into from

Conversation

jharshman
Copy link
Collaborator

Moves to resolve Issue #756

This is a small change set which allows the cobra generator to work outside of the GOPATH by setting the flag --vgo=<package>. If run without the --vgo flag, the generator flow remains the same.

Changes

  • Add a --vgo root persistent flag taking a string representing the package name.
    e.g: --vgo=github.com/spf14/hugo
  • If --vgo flag is set, allow init of project outside of GOPATH
  • For adding commands, if you are in the project directory when using --vgo, there is no need to specify package name. If you are outside the project directory, you must specify --package equal to the relative directory of your current working directory.

Future improvements

  • Cobra relies on the GOPATH to be able to derive the location of packages. I would like to see the weight on this dependency lighten as it is phased out of use in the language.
  • I think it's not needed to be able to add commands to projects from anywhere on the system. It's not a feature I really use and am curious if removing it would cause kickback from the community.

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2018

CLA assistant check
All committers have signed the CLA.

@andrewmeissner
Copy link

Hi @jharshman, it looks like circleci is reporting a failing check. the go tests pass on my local box with your changes. I'm not sure what else I might be missing, but from my end it should be passing. :/

@jharshman
Copy link
Collaborator Author

@andrewmeissner I'm not sure why there is a failing check there. Looks like it has something to do with bash completions. Will have to investigate this further.

@andrewmeissner
Copy link

@jharshman just wanted to check on this. Did anything show up in the bash completions investigation?

Thanks again!

@jharshman
Copy link
Collaborator Author

@andrewmeissner Hadn't had time to do take an in-depth look due to the holidays. I'll give it a go in the next couple days and see what turns up.

@jharshman
Copy link
Collaborator Author

jharshman commented Dec 15, 2018

@andrewmeissner ah I see the fix - it's a formatting error. I'll fix it real quick.
Update: All tests pass on my machine - seems to fail on formatting check which passes for me locally.

+diff -u bash_completions_test.go.orig bash_completions_test.go
+--- bash_completions_test.go.orig	2018-12-15 03:03:01.994127122 +0000
++++ bash_completions_test.go	2018-12-15 03:03:01.994127122 +0000
+@@ -71,7 +71,7 @@
+ 		ArgAliases:             []string{"pods", "nodes", "services", "replicationcontrollers", "po", "no", "svc", "rc"},
+ 		ValidArgs:              []string{"pod", "node", "service", "replicationcontroller"},
+ 		BashCompletionFunction: bashCompletionFunc,
+-		Run:                    emptyRun,
++		Run: emptyRun,
+ 	}
+ 	rootCmd.Flags().IntP("introot", "i", -1, "help message for flag introot")
+ 	rootCmd.MarkFlagRequired("introot")

756 - tweak vgo param to be string

* vgo param represents package name of project

756-vgo-support - create in current working directory

756-vgo-support - init outside of GOPATH

756-vgo-support - work on init cmd

formatting
@andrewmeissner
Copy link

Hi again, @jharshman , just wanted to follow up on this.

@tsal
Copy link

tsal commented Jan 23, 2019

This could be a valid workaround for #795 ?

@andrewmeissner
Copy link

@tsal seems like it. As long as cobra can work with modules - I'm a happy camper. :)

@jharshman
Copy link
Collaborator Author

@andrewmeissner #817
I re-worked this a bit more. Closing this PR

@jharshman jharshman closed this Jan 31, 2019
@andrewmeissner
Copy link

Awesome! Thanks @jharshman !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants