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

Fix command to deploy the simplest operator #34

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Sep 20, 2018

Signed-off-by: Georgios Andrianakis geoand@gmail.com

@jpkrohling
Copy link
Contributor

This change is Reviewable

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          16       16           
  Lines         599      599           
=======================================
  Hits          594      594           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb91503...aeef23e. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a 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, 1 unresolved discussion (waiting on @geoand)


CONTRIBUTING.adoc, line 52 at r2 (raw file):

[source,bash]
----
make build run

Calling build before run should not be required. Did you have problems when running run without build?

@geoand
Copy link
Contributor Author

geoand commented Sep 20, 2018

I did have a problem, since run attempts to invoke the built artifact (./_output/bin/jaeger-operator) which is not present if the project is not built beforehand.
Am I perhaps going about this the wrong way?

Thanks

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

You are absolutely correct. I just checked the Makefile and it does call the binary. A previous version probably executed go run main.go instead, which is what got me confused. Would you try just changing the Makefile to do go run main.go start instead of calling the binary and see if that works? One thing that might break is the "version" command, but other than that, it should just work.

If it works, then I'd appreciate if you could open another PR with that change alone, leaving this one without the make build run part.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @geoand)

@geoand
Copy link
Contributor Author

geoand commented Sep 20, 2018

@jpkrohling Sure thing, on it :)

Signed-off-by: Georgios Andrianakis <geoand@gmail.com>
@geoand geoand changed the title Fix contributing doc instructions about developing the operator Fix command to deploy the simplest operator Sep 20, 2018
@geoand
Copy link
Contributor Author

geoand commented Sep 20, 2018

@jpkrohling I updated this PR to include the simplest CRD command fix and opened #35 to address the run goal fix

Copy link
Contributor

@jpkrohling jpkrohling left a 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: :shipit: complete! all files reviewed, all discussions resolved

@jpkrohling jpkrohling merged commit 80e9a90 into jaegertracing:master Sep 20, 2018
@geoand geoand deleted the contributing-fix branch September 20, 2018 14:34
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.

2 participants