-
Notifications
You must be signed in to change notification settings - Fork 344
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
Fix the run goal #35
Fix the run goal #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
=======================================
Coverage 99.16% 99.16%
=======================================
Files 16 16
Lines 599 599
=======================================
Hits 594 594
Misses 5 5 Continue to review full report at Codecov.
|
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.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @geoand)
Makefile, line 56 at r1 (raw file):
run: crd @OPERATOR_NAME=$(OPERATOR_NAME) KUBERNETES_CONFIG=$(KUBERNETES_CONFIG) WATCH_NAMESPACE=$(WATCH_NAMESPACE) go run main.go start
Interesting: a side effect of this is that go run
will exit with a return code 1
, causing make
to complain about it:
^CINFO[0001] Jaeger Operator finished
make: *** [Makefile:56: run] Error 1
If it's not possible to get go run
to return the appropriate return code, it would probably make sense then to add something like || true
to the end of the command, so that make
is happy.
Sounds good, I'll take a look |
You might want also to add the |
Sure thing |
45649bf
to
a9ffdb3
Compare
@jpkrohling Issues should be addressed now :) |
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.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @geoand)
Makefile, line 56 at r2 (raw file):
run: crd bash -c 'trap "exit 0" INT; OPERATOR_NAME=${OPERATOR_NAME} KUBERNETES_CONFIG=${KUBERNETES_CONFIG} WATCH_NAMESPACE=${WATCH_NAMESPACE} go run -ldflags $(LD_FLAGS) main.go start'
I got to admit that it's close to magic to me :) Why are some vars wrapped in brackets and others in parenthesis? Also, are you developing on Mac OS or on Linux? I tested this on Linux and it works, but I wonder if would work the same on Mac OS.
A simpler version could be to just add "|| true" to the end, like (untested):
@OPERATOR_NAME=$(OPERATOR_NAME) KUBERNETES_CONFIG=$(KUBERNETES_CONFIG) WATCH_NAMESPACE=$(WATCH_NAMESPACE) go run -ldflags $(LD_FLAGS) main.go start || true
That said, this does LGTM. Once you confirm that it works on Mac OS, I'm ready to merge, unless you want to proceed with the "|| true" I mentioned.
@jpkrohling The parenthesis are wrong, I need to update to use brackets :) Unfortunately the |
a9ffdb3
to
1a1c7f5
Compare
I fixed the parenthesis issue. Also I am also on Linux, so I don't really know how this will behave on MacOS, although I would be surprised if it didn't work :P |
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.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @jpkrohling and @geoand)
Makefile, line 56 at r3 (raw file):
run: crd bash -c 'trap "exit 0" INT; OPERATOR_NAME=${OPERATOR_NAME} KUBERNETES_CONFIG=${KUBERNETES_CONFIG} WATCH_NAMESPACE=${WATCH_NAMESPACE} go run -ldflags ${LD_FLAGS} main.go start'
Sorry for not noticing this before, but it would be better to have a @
in front of the bash
command, so that it doesn't get printed out when doing a make run
:
$ make run
bash -c 'trap "exit 0" INT; OPERATOR_NAME=jaeger-operator KUBERNETES_CONFIG="/home/jpkroehling/.kube/config" WATCH_NAMESPACE=default go run -ldflags "-X "github.com/jaegertracing/jaeger-operator/pkg/version".commit=1a1c7f52ff549df06e0f475c1b951e9e29c64ea4 -X "github.com/jaegertracing/jaeger-operator/pkg/version".buildDate=2018-09-21T11:05:39Z -X "github.com/jaegertracing/jaeger-operator/pkg/version".defaultJaeger="1.6"" main.go start'
INFO[0000] Versions used by this operator: Version(Operator='1.6', GitCommit='1a1c7f52ff549df06e0f475c1b951e9e29c64ea4', BuildDate='2018-09-21T11:05:39Z', Jaeger='1.6', Go='go1.10.4', OperatorSDK='0.0.5+git')
INFO[0000] Metrics service jaeger-operator created
INFO[0000] Watching io.jaegertracing/v1alpha1, Jaeger, default, 5
^CINFO[0005] Jaeger Operator finished
vs.
$ make run
INFO[0000] Versions used by this operator: Version(Operator='1.6', GitCommit='1a1c7f52ff549df06e0f475c1b951e9e29c64ea4', BuildDate='2018-09-21T11:05:58Z', Jaeger='1.6', Go='go1.10.4', OperatorSDK='0.0.5+git')
INFO[0000] Metrics service jaeger-operator created
INFO[0000] Watching io.jaegertracing/v1alpha1, Jaeger, default, 5
^CINFO[0001] Jaeger Operator finished
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.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @geoand)
Instead of relying on the binary being present, we just use go run. We also set all the necessary compilation flags, environment variables and trap the interrupt signal in order to avoid having Make fail when the execution of the goal is interrupted Signed-off-by: Georgios Andrianakis <[email protected]>
1a1c7f5
to
c0e6b66
Compare
Ah yes, good catch! Fixed :) |
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.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @geoand)
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.
Reviewable status: complete! all files reviewed, all discussions resolved
Thanks! |
You're welcome! |
Instead of relying on the binary being present, we just use go run.
We also set all the necessary compilation flags, environment variables
and trap the interrupt signal in order to avoid having Make fail when
the execution of the goal is interrupted
Signed-off-by: Georgios Andrianakis [email protected]