-
Notifications
You must be signed in to change notification settings - Fork 2
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
Split examples into separate packages #14
Conversation
This change reduces the number of dependencies that the main package is required to import. Signed-off-by: Jim Fitzpatrick <[email protected]>
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.
Looks good!
One thing I'm noticing is that the with the seperate modules, the tests now in the examples
package is not run as part of the test workflow
You can see some tests are not run by running the following at the root directory and again at the examples
directory 🤔
go test -tags=unit,integration -v ./...
Ya, that is a good point, it never crossed my mind about the checks. I will add in two more jobs for testing the examples. |
Add two new jobs for testing the unit and integration test in the examples directory. Signed-off-by: Jim Fitzpatrick <[email protected]>
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.
Looks good to me 👍
@@ -1,2 +1,3 @@ | |||
bin | |||
topology.dot | |||
/kuadrant |
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.
?
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.
If you run go build
over using the make target you get the binary which I assume we never want included. I would be happy to remove. The leading / was added by goland, that is possibly not required, now that I look at it again.
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.
The example Makefile doesn't have a build
target. In case we add one, we can make it PHONY and it won't spit out the binary.
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.
I know there is no Makefile build
target. I ran go build
directly. This was partly habit but also setting a run target in Goland so I could attach a debugger process. I can remove it there is no problem.
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.
It's OK. I was just trying to understand. In the end, you're right about not committing the binary, so no harm in keeping it.
closes: #13
This change reduces the number of dependencies that the main package is required to import.