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

V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' #2182

Merged
merged 12 commits into from Jul 10, 2019

Conversation

@ikawaha
Copy link
Contributor

commented Jul 6, 2019

Fix issue #2181

@ikawaha ikawaha changed the title Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' Jul 6, 2019

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Thank you for the great issue report and the PR! A couple of questions:

  1. Would it make sense to search for "go.mod" recursively going up the directories to support the case where a single Go module hosts multiple Goa projects, e.g.:
root
├── go.mod
├── project1
│   └── design
└── project2
    └── design

Here the goa command would be run within project1 and project2 and it would be nice if it could pickup the go.mod file under root like the go command does.

  1. Instead of adding a dependency on a third party package ("github.com/sirkon/goproxy/gomod") would it be better to just copy the modfile package from the Go source code: https://github.com/golang/go/tree/master/src/cmd/go/internal/modfile. It's unfortunate that it is an internal package but it seems better to copy and give proper attribution than to add a dependency on another package.

Thank you again, this is a great contribution!

@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Thank you for your comment !

  1. The location of go.mod to be used can be found by go env.
    (cf. https://dave.cheney.net/2013/06/14/you-dont-need-to-set-goroot-really)
$ echo $GOMOD

$ go env GOMOD
/Users/ikawaha/go/src/github.com/ikawaha/calc/go.mod

So, I fixed to use go env MOD instead of "go.mod".

  1. That's right.
    However, go/internal/modfile (It can not be used alone. I think there needs modfetch, modload etc) seems a bit complicated. I think that it is best to be porting go/internal, but how about using this package until then?
@raphael
Copy link
Member

left a comment

Looks good, thanks

@@ -142,16 +142,34 @@ func (g *Generator) Write(debug bool) error {
return err
}

const GOMODENVKEY = "GOMOD"

func findGoMod() (string, error) {

This comment has been minimized.

Copy link
@raphael

raphael Jul 7, 2019

Member

Looks like this function can never return an error which is good so the error return value can be removed.

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Thinking a bit more about parsing go.mod: we really don't need a full parser. All we need is to find the line that starts with goa.design/goa/v3 and capture what's after. Seems a little heuristic here might be better than bringing in all these dependencies. Thank you!

@ikawaha ikawaha changed the title V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' [WIP]V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' Jul 7, 2019

@ikawaha ikawaha force-pushed the ikawaha:fix/v3/go-mod-require branch from bcce715 to da4ef14 Jul 7, 2019

@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

I replaced the third party package with a heuristic parser.
Does it make sense?

@ikawaha ikawaha changed the title [WIP]V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' V3: Fix to use the version of goa specified in go.mod file, when 'goa gen'/'goa example' Jul 7, 2019

@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

It may be better to output the version of goa when go get. 🤔

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

The parser looks great! Thank you for making the change. I'm happy to merge the PR as is.

ikawaha added 2 commits Jul 8, 2019
@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

That's all.

go.mod Outdated
@@ -13,6 +13,7 @@ require (
github.com/manveru/gobdd v0.0.0-20131210092515-f1a17fdd710b // indirect
github.com/pkg/errors v0.8.1
github.com/sergi/go-diff v1.0.0
github.com/sirkon/goproxy v1.4.0

This comment has been minimized.

Copy link
@ikawaha

ikawaha Jul 8, 2019

Author Contributor

Oops

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I wonder if we need to print the version on the console? this may break scripts that wrap goa and capture its output.

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Thank you!

@raphael raphael merged commit 59098d1 into goadesign:v3 Jul 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

What about displaying the version of goa modules by 'go get', when 'goa version' command run? If it looks good, I create another pull request.

@ikawaha ikawaha deleted the ikawaha:fix/v3/go-mod-require branch Jul 10, 2019

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Not sure about goa version but it would probably be OK to show it when running with the --debug flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.