-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add an alternate import syntax with better support of Go modules #32
Add an alternate import syntax with better support of Go modules #32
Conversation
} | ||
|
||
err := ExecCmd(exec.Command("go", "get", "-u", dep), m.srcDir) |
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.
go get
can only fetch HEAD of a repository (master).
We need another solution (fully-)based on Go modules, except for legacy contribs.
b6f48c7
to
fe7450a
Compare
fe7450a
to
48e9306
Compare
a2120e3
to
ec21f84
Compare
I added a support for "legacy" imports which does not require project-flogo/core#44 new syntax Legacy flavourflogo.jsonflogo.json imports with old syntax:
Steps
Resulting go.mod file
Some indirect dependencies exist (including github.com/project-flogo/contrib which should be direct). Module flavour (preferred)flogo.jsonflogo.json imports with new syntax (project-flogo/core#44):
Steps
Resulting go.mod file
Notice it is less verbose than the legacy one and each dependency is explicitly and directly imported. |
74d6271
to
add696b
Compare
So is the issue that flogo install github.com/square-it/flogo-opentracing-listener@v0.0.2 uses go get under the covers? Is the only advantage mod require doesn't add the indirect imports? I think if you do Also when you create a project, it should create a go.mod by default, so not sure why anything would fail due a go.mod file not being there (since it should be there). @skothari-tibco What are your thoughts on this? I do like adding to imports on install, but we might have to add a "tidy" to remove imports to unused triggers and activities. |
I do like the code cleanup in general for handling the imports. I'm cool with adopting this if it works as well as before. @skothari-tibco thoughts? @mellistibco thoughts on adding all import to the flogo.json? I kind of like this, makes it clear as to what you are using, especially if you aren't a go coder. Maybe we can add a "tidy" like command that can prune any triggers or activities that aren't directly referenced. One question I have is about the new contrib import format: Why this |
b3f3598
to
f599cab
Compare
f599cab
to
78b3606
Compare
Hey @debovema , I tried the new cli with the steps you specified. It was super simple to follow the steps, thanks. So I tied
Am I missing something? Is there a different way to install the contrib? Or is this the expected behavior? |
Thank for your replies @fm-tibco. I will try to answer to all your questions and explain everything as it has moved a lot since first commit on this PR. First the issue is not only that it uses
Then, I agree that a project should always have a go.mod file. You might have misunderstood something in previous comments. I now agree that About your concerns around compatibility, I tried to evolve the new model to support both approach. There are two changes to take care of though, both living in project-flogo/core#44 for now:
The step-by-step guide can help you validate quickly the good behaviour of this PR in a Docker container. It is working for |
@skothari-tibco : you're not missing anything ! :) This is the expected behaviour. If you want to install your contribution with the new syntax, simply use The runtime support of all kinds of imports in the imports array in flogo.json is brought by project-flogo/core#44 (see comment above).
UPDATE: in fact, this cannot be avoided since there is no way to distinguish between github.com/skothari-tibco/flogo-components/activity/twillio and github.com/skothari-tibco/flogo-components:/activity/twillio since that would imply to know where the module lives when using github.com/skothari-tibco/flogo-components/activity/twillio and that's what I wanted to address in this PR. |
I think the reason the code is currently adding an indirect import is because it does the go get before creating the actual dependency in the import file. If before tidy:
after tidy
|
@skothari-tibco : updated previous comment @fm-tibco : you are certainly right and I think the right place to do this However that does not change the failing behaviour when using |
As for go mod vs go get plumbing, I'm not 100% convinced one is better than the other at the moment. It would be nice if the Go guys would un-blur the lines between them. I agree with Nate Finch in that go development discussion I referred to above. Regardless I'm fine with using 'go mod' if things will be more clean and consistent. As for the other advantages, I think we could sort contributions by name and version regardless of the ':' syntax. I do agree that the ':' does have an advantage in avoiding a network call or any extra disk calls. The best we could do to optimize the current behavior is read-in the existing go.mod and see if a parent module exists, and then avoid the I'm fine if we support that syntax for the advanced user, but for lot of users they are probably just going just add the import for the specific thing they want and not try to determine where the mod file is located relative to the import they are doing. So just to be clear the imports:
are the same, they both get the latest tagged version
are the same, they both get the master
are the same, they both get the 1.0.0 version. Btw, I think we should support with and without the v, in my opinion the v is kind of redundant... but I know its in the tag and some people use it that way. |
Yes, your three examples are correct except the version should be before the : However I disagree with dropping the v in the versions as I think it is part of the Go modules "specification". So fixed examples would look like:
|
I'm not saying drop support for it, but I do think we should allow Which is what other apps support, like npm for example... I normally don't think I need to add a 'v' when installing version 1.0.0 of something.. you just do |
OK. I think we should delay this feature of having v as an option in a next PR, don't you think ? 😆 |
@debovema I do understand now. This |
As I said on Gitter, I think we should use the |
We need to confirm the following with the new approach:
As for the open tracing listener, it works with go get in my environment, not sure why it doesn't work for you. According to the go documentation they expect users to use |
@fm-tibco : I will check this and update this PR accordingly |
d189af8
to
5fb3472
Compare
5fb3472
to
7936447
Compare
@fm-tibco : it was not such a big modification since the @latest is working the same way in See especially this commit 4f64254 added today. I did many tests mixing different syntaxes, different versions (with explicit versions, branches, latest...) and everything seems to be very fine. This PR description was also reworked to speak of classic syntax and alternate syntax and emphasize the fact that only users who want to opt in will be concerned by these changes. If you still have questions, don't hesitate to ask me here or in Gitter. If it's OK, merge order would be:
Thanks again for your time everyone. |
@debovema I tried |
Considering the module also depends on github.com/project-flogo/flow@v0.9.0-alpha4 and github.com/project-flogo/contrib@v0.9.0-alpha4 and that these modules both depend on github.com/project-flogo/core@v0.9.0-alpha4, the odd thing is that it's using v0.9.0-alpha.5 not v0.9.0-alpha.4 |
I think the fact that it gets the latest available v0.9.0 is caused by the I did the following test
Correct because v0.9.0-alpha.3 would not satisfy the dependencies of flow and contrib modules in their v0.9.0-alpha.4 versions. |
Wouldn't this create confusion? Also we may need |
@skothari-tibco: I created issue #35 to describe this and #36 to fix it. |
Abstract
This PR improves the way Flogo CLI manage imports of contributions:
flogo install
parametersflogo install
will also be added in flogo.json file in imports array.go mod
plumbering when it's possible, withgo get
when it's notRequirements
Tests
Step-by-step guide
To test this fork with fork from project-flogo/core#20 & project-flogo/core#44:
a. use updated flogo.json with alternate import syntax from PR project-flogo/core#44 to create a new app (classic syntax is obviously still supported, the only difference is that alternate syntax will use
go mod
instead ofgo get
plumbering)b. otherwise create directly a standard app with classic syntax
a. if project was created using new syntax, override go.mod to use project-flogo/core#44
b. otherwise
One-liner test command
The test above can be run with the CLI from master then the CLI from this fork using this command:
Details of execution are in this gist.