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

Support mod feature #43

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

GrantZheng
Copy link

1 Supporting the go mod feature, you can create the project outside the gopath with --mod_module flag
2 Optimizing redundant code

cmd/g_service.go Outdated Show resolved Hide resolved
cmd/g_service.go Outdated Show resolved Hide resolved
@kujtimiihoxha
Copy link
Owner

@GrantZheng It will take some time for me to review this but thanks a lot for introducing this feature.

@GrantZheng
Copy link
Author

@GrantZheng It will take some time for me to review this but thanks a lot for introducing this feature.

@kujtimiihoxha Hi, how about the feature review :)

@kujtimiihoxha
Copy link
Owner

@GrantZheng thanks a lot for this PR, I will see if I can find some time this weekend and will go through your PR 👍

@kujtimiihoxha
Copy link
Owner

@GrantZheng I tested this and it does not seem to behave as it should.

  1. The module name is whatever the user specifies, it could be something like my_project, it does not have to contain the github.com path.
  2. It does not matter where the folder gets created.
  3. The command should create the go.mod file with the module name
  4. We probably need a way to autodetect if the project is using gomod and use the module name from go.mod when generating the service.

I also tried the example and it did not generate the service for me.

@GrantZheng
Copy link
Author

@GrantZheng I tested this and it does not seem to behave as it should.

  1. The module name is whatever the user specifies, it could be something like my_project, it does not have to contain the github.com path.
  2. It does not matter where the folder gets created.
  3. The command should create the go.mod file with the module name
  4. We probably need a way to autodetect if the project is using gomod and use the module name from go.mod when generating the service.

I also tried the example and it did not generate the service for me.

@kujtimiihoxha Thank you very much for your suggestion. I will achieve it as soon as possible. :)

@GrantZheng
Copy link
Author

@GrantZheng I tested this and it does not seem to behave as it should.

  1. The module name is whatever the user specifies, it could be something like my_project, it does not have to contain the github.com path.
  2. It does not matter where the folder gets created.
  3. The command should create the go.mod file with the module name
  4. We probably need a way to autodetect if the project is using gomod and use the module name from go.mod when generating the service.

I also tried the example and it did not generate the service for me.

@kujtimiihoxha Hi,according to these suggestion , I commit the PR again and have made these changes:

  1. You could create a new service outside the gopath with --mod_module flag, and it will generate go.mod file with the module name, such as
    kit n s hello --mod_module hello
  2. When you are generating the service and the client library, the module name in the go.mod file could be autodetected.
  3. I replace the "github.com/openzipkin/zipkin-go-opentracing" by "github.com/openzipkin-contrib/zipkin-go-opentracing"
    Thanks a lot for taking time to review it :)

@keets2012
Copy link

how about mod support? @GrantZheng @kujtimiihoxha

@GrantZheng
Copy link
Author

GrantZheng commented Oct 15, 2019

how about mod support? @GrantZheng @kujtimiihoxha

Hi @keets2012 , the PR has not been checked,you can try it on the branch of https://github.com/GrantZheng/kit/tree/feature/mod_support . I am very glad to recieve any suggestion obout the PR.

@kujtimiihoxha
Copy link
Owner

@GrantZheng I will get to this as soon as I can 👍

@GrantZheng
Copy link
Author

@GrantZheng I will get to this as soon as I can

@kujtimiihoxha Hi,how about it :)

@kujtimiihoxha
Copy link
Owner

@GrantZheng Thanks a lot for making the requested changes, I am really sorry it is taking me so much to review these.

How the command now works for each service that we create it will create a separate mod file for them.

My suggestion would be that we introduce a new command something like:

kit new project {name}

If we run something like

kit new project hello

This command would setup a folder structure like this:

hello/
-----> go.mod

Now when you cd into this directory you can run the normal command like

kit generate service {name}

But now because we already have a mod file we do not need --mod_module flag.

It should be pretty straight forward to change this since you already have done the hard part.

Let me know if you like the idea here.

Copy link
Owner

@kujtimiihoxha kujtimiihoxha left a comment

Choose a reason for hiding this comment

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

Some small changes

generator/generate_service.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@GrantZheng
Copy link
Author

GrantZheng commented Dec 15, 2019

@GrantZheng Thanks a lot for making the requested changes, I am really sorry it is taking me so much to review these.

How the command now works for each service that we create it will create a separate mod file for them.

My suggestion would be that we introduce a new command something like:

kit new project {name}

If we run something like

kit new project hello

This command would setup a folder structure like this:

hello/
-----> go.mod

Now when you cd into this directory you can run the normal command like

kit generate service {name}

But now because we already have a mod file we do not need --mod_module flag.

It should be pretty straight forward to change this since you already have done the hard part.

Let me know if you like the idea here.

It's a good idea. But if we do not need '--mod_module' flag, the module name in the go.mod file is the project {name}, then it uses the module name from go.mod when generating the service. So that, the import path in other files is relative project path, such as :

import (
     service "hello/pkg/service"
)

We'd better generate the import package with relative repository path, such as :

import (
	service "github.com/GrantZheng/hello/pkg/service"
)

When the third party project is dependent on your project, the "hello/pkg/service" package may not be easy to find.

@kujtimiihoxha
Copy link
Owner

@GrantZheng good point we might want to allow them to specify the module.

So we could have something like

kit new project my_proj --module "github.com/GrantZheng/my_project" 

By default if --module is not provided we use the project name.

@GrantZheng
Copy link
Author

GrantZheng commented Dec 15, 2019

@GrantZheng good point we might want to allow them to specify the module.

So we could have something like

kit new project my_proj --module "github.com/GrantZheng/my_project" 

By default if --module is not provided we use the project name.

@kujtimiihoxha Good idea, I quite agree with you :)

…place lightsteptracergo.FlushLightStepTracer with lightsteptracergo.Flush
@GrantZheng
Copy link
Author

@kujtimiihoxha Hi, I have solved obove problems, please review it again. :)

@kujtimiihoxha
Copy link
Owner

@GrantZheng thanks for the changes.

Could you reset go.sum I think these changes are accidental.

Copy link
Owner

@kujtimiihoxha kujtimiihoxha left a comment

Choose a reason for hiding this comment

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

I am not being able to run the service after it get's created, the issue with the way it is implement is that the go.mod file needs to sit at the parent level not at the service level if we want to maintain multiple services under the same package.

I created a folder called my_project then I ran

kit n s add --module github.com/kujtimiihoxha/my_project/add 

After that I ran

 kit g s add  

And when I try to run

go run add/cmd/main.go

I am getting
Screenshot 2020-01-19 at 5 07 59 PM

Here is the folder structure
Screenshot 2020-01-19 at 5 07 46 PM

README.md Outdated
@@ -17,6 +17,7 @@ GoKit Cli needs to be installed using `go get` and `go install` so `Go` is a req
- [Generate the service](#generate-the-service)
- [Generate the client library](#generate-the-client-library)
- [Generate new middlewares](#generate-new-middleware)
- [Mod feature support](#mod-feature-support)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- [Mod feature support](#mod-feature-support)
- [Module support](#mod-feature-support)

@@ -53,3 +60,19 @@ func (g *NewService) Generate() error {
)
return g.fs.WriteFile(g.filePath, g.srcFile.GoString(), false)
}

func (g *NewService) genModule() error {
exist, _ := g.fs.Exists(g.name + "/go.mod")
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we want to add the dependencies manually, at least the once we depend on after running kit g s

Copy link
Author

@GrantZheng GrantZheng Jan 20, 2020

Choose a reason for hiding this comment

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

Hi @kujtimiihoxha :)

Why we need to maintain multiple services under the same package?
I think it has two scenes:
The one scene is that we just maintain multiple services and convenient to manage the project. There have no relation between services and services are started by there own main.go file ({service}/cmd/main.go).

The other scene is that we want to construct a monolithic project that contains multiple services under the same package. There have some dependency between services and the monolithic project is started only by one main.go file which needs to be create manually at the service's parent level. In addition, we should also delete the go.mod file at the service level and create a go.mod file manually at the parent level.

About the one scene, I think it is of small significance and rarely used in this way. We only need to cover most of the usage scenarios. There's no need to support.

About the other scene, I think it is significant. But I do not recommend realizing the function at the kit tool. The responsibility of kit tool is just construct our services. If we want to construct a monolithic project, we should do manually as follows:

  1. delete the go.mod file at the service level and create a go.mod file manually at the parent level
  2. create a main.go file at the service's parent level
    But, when we need to add functions to a service , the kit g s {service} will not work well, because we delete the go.mod file under the service level, the module name in the go.mod file can not be autodetected. To solve this problem, the kit tool should add a function that if the go.mod file can not be found in the service level, it will use the go.mod file in parent level to generate service and client library.

What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

I think you are right.

One other thing that came to my mind now is that with how it is implemented now we are not maintaining backwards compatibility with already existing projects that use kit.

Is there a way to only use modules when the user wants to do that ?

Basically only use mod if --module is provided OR the go.mod file is detected in the service folder.

Copy link
Author

Choose a reason for hiding this comment

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

About compatible with the already existing projects,we may have two ways, I think.

The one is that we should use tag vesion to manage our feature, such as checkout two new branches named release/v0.1.0 and release/v0.2.0 from the master and tag the release/v0.1.0 branch code v0.1.0 which should run kit commands under the GOPATH, and then switch to release/v0.2.0 branch, merge code from feature/mod_support branch, tag v0.2.0 under the release/v0.2.0 branch. If the user want to use the GOPATH ,he can use the v0.1.0 version; if the user want to use the module feature, he can use the v0.2.0 version.

The other is that continue to support GOPATH in our new version, if user run kit n s {service} without module flag, it will use the old way to new and generate service and client library.

I think the first way is better than the second. I want to quote some word from https://blog.golang.org/modules2019.

Our aim is for Go 1.13, scheduled for August 2019, to enable module mode by default (that is, to change the default from auto to on) and deprecate GOPATH mode. In order to do that, we’ve been working on better tooling support along with better support for the open-source module ecosystem.

So, I think there is no need to compatible with GOPATH in our new version. We should guide the user to use the new module feature. If the user have to use the GOPATH, he could switch to the old version v0.1.0.

What do you think about this? :)

@GrantZheng
Copy link
Author

@GrantZheng thanks for the changes.

Could you reset go.sum I think these changes are accidental.

Oh, sorry, I have reset the go.sum, please check again, thanks a lot

"github.com/lightstep/lightstep-tracer-go", "FlushLightStepTracer",
).Call(jen.Id("tracer")),
"github.com/lightstep/lightstep-tracer-go", "Flush",
).Call(jen.Id("context.Background()"), jen.Id("tracer")),
Copy link
Owner

@kujtimiihoxha kujtimiihoxha Jan 20, 2020

Choose a reason for hiding this comment

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

You need to use Qual here context is missing in the imports.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I fix it and update the pr

@juanpablopizarro
Copy link

@kujtimiihoxha have you an ETA to merge this?, your tool is great but I'm facing some issues:
cmd/service/service.go:58:21: undefined: zipkintracer.NewHTTPCollector
cmd/service/service.go:64:15: undefined: zipkintracer.NewRecorder
cmd/service/service.go:65:17: undefined: zipkintracer.NewTracer

@GrantZheng
Copy link
Author

@kujtimiihoxha have you an ETA to merge this?, your tool is great but I'm facing some issues:
cmd/service/service.go:58:21: undefined: zipkintracer.NewHTTPCollector
cmd/service/service.go:64:15: undefined: zipkintracer.NewRecorder
cmd/service/service.go:65:17: undefined: zipkintracer.NewTracer

@juanpablopizarro Hi, I have fixed it on my own repo (https://github.com/GrantZheng/kit/releases), you could try to use it until the author merges the PR.

@juanpablopizarro
Copy link

Great. Yesterday I did a test and i got some issues with the imports, but nothing serious. Thanks.

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