ponzu add <addon URI> #87

olliephillips · 2017-02-24T19:22:20Z

Reference issue #85. As discussed, this adds a ponzu add command to the CLI. It fetches the addon from its URI by wrapping go get then copies it to the projects local ./addons directory.

There is no switch for automatically installing. Based on the issue discussion that feature needs more thought, so has been omitted.

I've updated the README to include documentation on the command.

It's been tested as per the approach in the README.

…rob don't need mkdir

…ATH/src

nilslice · 2017-02-24T19:42:24Z

cmd/ponzu/add.go

+
+	// Go get
+	cmdOptions = append(cmdOptions, "get", addonPath)
+	get := exec.Command(gocmd, cmdOptions...)


good catch to re-use the gocmd variable.. will be very useful when working with future go version betas / release candidates. thank you for being mindful about that!

nilslice · 2017-02-24T19:44:48Z

cmd/ponzu/add.go

+	// copy to ./addons folder
+	// GOPATH can be a list delimited by ":" on Linux or ";" on Windows
+	// `go get` uses the first, this should parse out the first whatever the OS
+	gopath := resolveGOPATH()


if you don't mind, before we merge this in, could you take a look at #82 and see how it would impact GOPATH resolution in the way you have it?

#82 hasn't been fixed, but the code is pretty much written in the issue... if you felt inclined to add that in I would be grateful.

Certainly. I didn't know anything about that. I only upgraded to 1.8 to run Ponzu :)

Great -- thank you very much!

nilslice · 2017-02-24T19:58:41Z

cmd/ponzu/usage.go

@@ -162,6 +162,19 @@ var usageVersion = `

 `

+var usageAdd = `
+[--cli] add, a <addon URI>


is [--cli] actually used in the add command? if not, lets clear that so it's not confusing. then in place of , let's say

It's not in the end PR, I spotted it late and removed it in [this commit] (5fcca31)

All good! Thank you. I figured it was a copy/paste thing.. and I appreciate you keeping to the style of the existing commands!

nilslice

@olliephillips - this is fantastic. thank you for the work you put here. if you could make the edits as per the comments and lmk what you think about GOPATH resolution since 1.8 changes it will help a lot to get this merged. I am on the road until later tonight, but will be able to check in if you have issues.

olliephillips · 2017-02-27T13:41:29Z

This is what I had in mind for GOPATH resolution

// resolve GOPATH. In 1.8 can be default, or custom. A custom GOPATH can
// also contain multiple paths, in which case 'go get' uses the first
func getGOPATH() (string, error) {
	var gopath string
	gopath = os.Getenv("GOPATH")
	if gopath == "" {
		// not set, find the default
		usr, err := user.Current()
		if err != nil {
			return gopath, err
		}
		gopath = filepath.Join(usr.HomeDir, "go")
	} else {
		// parse out in case of multiple, retain first
		gopaths := strings.Split(gopath, ":") // Linux
		gopath = gopaths[0]
		gopaths = strings.Split(gopath, ";") // Windows
		gopath = gopaths[0]
	}
	return gopath, nil
}

sirikon · 2017-02-27T18:31:49Z

@olliephillips That script could have the same problem I commented you in the code.

nilslice · 2017-02-27T18:50:56Z

@sirikon - could you link me to the comment / line in the code you mention? I can't find it.

sirikon · 2017-02-27T20:26:14Z

@nilslice https://github.com/ponzu-cms/ponzu/pull/87/files/88dc9b4d26022aeda295e03690c4eb4398e40487#diff-88cf37571f8025a3c6cc4fa53928c999R62

nilslice · 2017-02-27T20:43:58Z

@sirikon - sorry, I meant the link to your comment that you left about:

That script could have the same problem I commented you in the code.

I don't see any comment there, maybe I'm just missing it..

sirikon · 2017-02-28T08:34:02Z

@nilslice This one you mean? https://github.com/ponzu-cms/ponzu/pull/87/files#r103056524

nilslice · 2017-02-28T10:40:52Z

@sirikon - odd, this is what I see when clicking your link. No comment from you on the whole PR..

Would you mind copying the comment you made and the line # you're referencing so I can understand what the issue is?

Thanks & sorry for the inconvenience.. really not sure why I can't see the comment

sirikon · 2017-02-28T10:44:56Z

For god sake Github X_X.

Sorry guys, here it is: Line 62-69 of file add.go

olliephillips · 2017-02-28T10:56:18Z

@sirikon That's a good point re : in windows path. Probably this could work without OS check if ; checked first and then branch out - so no second test. I'll look at that.

Before I do could I have comments on the revised func above, which attempts to resolve a default GOPATH.

sirikon · 2017-02-28T10:57:26Z

The rest of the script seems perfect for me :)

olliephillips · 2017-02-28T14:27:15Z

Needed an OS check in end. Hopefully, this should reliably get GOPATH or the default. At least for Windows/Linux/Mac. If agreed I'll update the PR.

// resolve GOPATH. In 1.8 can be default, or custom. A custom GOPATH can
// also contain multiple paths, in which case 'go get' uses the first
func getGOPATH() (string, error) {
	var gopath string
	gopath = os.Getenv("GOPATH")
	if gopath == "" {
		// not set, find the default
		usr, err := user.Current()
		if err != nil {
			return gopath, err
		}
		gopath = filepath.Join(usr.HomeDir, "go")
	} else {
		// parse out in case of multiple, retain first
		if runtime.GOOS == "windows" {
			gopaths := strings.Split(gopath, ";")
			gopath = gopaths[0]
		} else {
			gopaths := strings.Split(gopath, ":")
			gopath = gopaths[0]
		}
	}
	return gopath, nil
}

sirikon · 2017-02-28T14:28:37Z

Seems okay for me :)

nilslice · 2017-02-28T21:14:56Z

@sirikon - thank you for adding that, sorry it was pending.. not sure how to approve those things

@olliephillips - that looks good to me, go ahead and update the PR at your convenience.

Thanks!

olliephillips · 2017-03-01T10:35:28Z

@nilslice That has been added to the PR. I've run through the ponzu build process again and seems ok to me

sirikon · 2017-02-24T23:44:52Z

cmd/ponzu/add.go

+	gopaths = strings.Split(envGOPATH, ";")
+	gopath = gopaths[0]
+	return gopath
+}


Does this method work correctly?
(Didn't knew GOPATH could be multiple directories by the way, interesting)

I mean... This will only work in Windows, because 'gopath' var is overwritten.

Also, making the split by ":" and by ";" without checking what's the OS Host is tricky, for example, in Windows ":" is used to define drive letters (Like in the path C:\Users\Whatever)

nilslice

Thank you! Looks good. Will just run through some tests and merge.

nilslice · 2017-03-01T18:46:10Z

@olliephillips, thank you for adding this. I think it will be a really big help to make addons a major component of Ponzu systems.

@sirikon, thank you for your continued involvement and testing!

I really appreciate all of your help.

Ollie Phillips added 16 commits February 24, 2017 15:17

add as command line option and usage text

fff0a34

Added usageAdd to useage const

bf58736

add.go added

7fbc2ae

getAddon(), comprising go get and copy routines

586e877

Comment improvement

0c16e95

added add.go to cmdBuildFiles in buidPonzuServer()

58e3755

missed get in exec.command

522eab3

break GOPATH determination out into separate function

cdc044e

Some logging, files not copied

bf695fb

missed present directory from dest path, removed logging

7a11bbc

was thinking copyAll handled mkdir

dd03930

missing src in the src path, nothing wrong with the destination and p…

28c360a

…rob don't need mkdir

Implemented a replicateAll func, since we need files to remain in GOP…

2884d7d

…ATH/src

Added ponzu add to the README

121a63a

Removed some redundant info I missed

5fcca31

README tweak

88dc9b4

nilslice self-requested a review February 24, 2017 19:33

nilslice reviewed Feb 24, 2017

View reviewed changes

nilslice suggested changes Feb 24, 2017

View reviewed changes

revised GOPATH detection for bug and Go 1.8 default

b0975d1

sirikon approved these changes Mar 1, 2017

View reviewed changes

nilslice approved these changes Mar 1, 2017

View reviewed changes

update usage

7297805

nilslice merged commit 69d55ee into ponzu-cms:ponzu-dev Mar 1, 2017

This was referenced Mar 1, 2017

Addon setup - ponzu get github.com etc #85

Closed

Unexpected behaviour creating a ponzu project #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ponzu add <addon URI> #87

ponzu add <addon URI> #87

olliephillips commented Feb 24, 2017 •

edited

Loading

nilslice Feb 24, 2017

nilslice Feb 24, 2017

olliephillips Feb 24, 2017

nilslice Feb 24, 2017

nilslice Feb 24, 2017

olliephillips Feb 24, 2017 •

edited

Loading

nilslice Feb 24, 2017

nilslice left a comment

olliephillips commented Feb 27, 2017

sirikon commented Feb 27, 2017

nilslice commented Feb 27, 2017

sirikon commented Feb 27, 2017

nilslice commented Feb 27, 2017

sirikon commented Feb 28, 2017

nilslice commented Feb 28, 2017

sirikon commented Feb 28, 2017

olliephillips commented Feb 28, 2017

sirikon commented Feb 28, 2017

olliephillips commented Feb 28, 2017

sirikon commented Feb 28, 2017

nilslice commented Feb 28, 2017

olliephillips commented Mar 1, 2017

sirikon Feb 24, 2017

sirikon Feb 24, 2017

nilslice left a comment

nilslice commented Mar 1, 2017

ponzu add <addon URI> #87

ponzu add <addon URI> #87

Conversation

olliephillips commented Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliephillips Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilslice left a comment

Choose a reason for hiding this comment

olliephillips commented Feb 27, 2017

sirikon commented Feb 27, 2017

nilslice commented Feb 27, 2017

sirikon commented Feb 27, 2017

nilslice commented Feb 27, 2017

sirikon commented Feb 28, 2017

nilslice commented Feb 28, 2017

sirikon commented Feb 28, 2017

olliephillips commented Feb 28, 2017

sirikon commented Feb 28, 2017

olliephillips commented Feb 28, 2017

sirikon commented Feb 28, 2017

nilslice commented Feb 28, 2017

olliephillips commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilslice left a comment

Choose a reason for hiding this comment

nilslice commented Mar 1, 2017

olliephillips commented Feb 24, 2017 •

edited

Loading

olliephillips Feb 24, 2017 •

edited

Loading