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

Moving location of statuszTmpl #206

Merged
merged 9 commits into from
Feb 5, 2020
Merged

Conversation

fifiug
Copy link
Contributor

@fifiug fifiug commented Nov 28, 2019

When trying to run $GOPATH/bin/stackdriver-prometheus-sidecar --help ,I get error panic: open statusz-tmpl.html: no such file or directory.
Moving the intialization of the statuszTmpl to func ServeHTTP which allows the use of --help

When trying to run $GOPATH/bin/stackdriver-prometheus-sidecar --help ,I get error panic: open statusz-tmpl.html: no such file or directory.
Moving the intialization of the statuszTmpl to func ServeHTTP which allows the use of --help
@StevenYCChou StevenYCChou self-assigned this Dec 2, 2019
Copy link
Contributor

@StevenYCChou StevenYCChou left a comment

Choose a reason for hiding this comment

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

I don't think this one solve the real issue for a locally built binary - If the locally built binary runs the function ServeHTTP, it will panic again.

Potential solutions is to either package the statusz-tmpl.html into the binary, or use a const/variable to hold the template directly rather than parsing from files.

@fifiug fifiug changed the title Moving var statuszTmpl to func ServeHTTP Moving location of statuszTmpl Dec 6, 2019
@fifiug
Copy link
Contributor Author

fifiug commented Dec 6, 2019

I decided to go with using a const that holds the template since I wasn't able to find a way to build the template into a go binary. Please let me know if that commit works

Copy link
Contributor

@StevenYCChou StevenYCChou left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to hear some thoughts from @jkohen as he requested having the template into its own file before.

cmd/stackdriver-prometheus-sidecar/statusz.go Outdated Show resolved Hide resolved
@igorpeshansky
Copy link
Member

Maybe this?

Copy link
Contributor

@jkohen jkohen left a comment

Choose a reason for hiding this comment

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

Thank you, it looks great. JUst one comment to address, please.

cmd/stackdriver-prometheus-sidecar/statusz.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jkohen jkohen left a comment

Choose a reason for hiding this comment

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

@jkohen moved it to the init method. I have a a question using promu to build the binary. When I run "go build" in the directory "cmd/stackdriver-prometheus-sidecar", the generated binary runs as expected. However when I use the Makefile that runs the "promu build", the binary generated is against the old version of code(I can tell because of the error message and line number). Is this WAI or I'm running promu wrong ?

I see in the Travis presubmit results that the build is failing due to a missing dependency in the vendor directory. If you are running go build without -mod=vendor, then the compiler is picking up the statik package from your local workstation. The makefile uses -mod=vendor, so it's failing to build the binary, and I assume you're running a pre-existing binary.

You should be able to fix this by running go mod vendor.

cmd/stackdriver-prometheus-sidecar/statusz.go Outdated Show resolved Hide resolved
@jkohen
Copy link
Contributor

jkohen commented Jan 28, 2020

LGTM. @StevenYCChou you own this review, please merge when you're satisfied with it.

@StevenYCChou
Copy link
Contributor

@fifiug can you ensure the test passes?

@fifiug
Copy link
Contributor Author

fifiug commented Feb 3, 2020

@StevenYCChou test passed. My pull request can't access the COVERALLS_TOKEN so "make goveralls" fails

Copy link
Contributor

@StevenYCChou StevenYCChou left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution, @fifiug !

@StevenYCChou StevenYCChou merged commit 2548e8a into Stackdriver:master Feb 5, 2020
msukruth pushed a commit to msukruth/stackdriver-prometheus-sidecar that referenced this pull request Jun 2, 2020
* Moving var statuszTmpl to func ServeHTTP

When trying to run $GOPATH/bin/stackdriver-prometheus-sidecar --help ,I get error panic: open statusz-tmpl.html: no such file or directory.
Moving the intialization of the statuszTmpl to func ServeHTTP which allows the use of --help

* Moving statusz-tmpl.html to a const in statusz.go and updating related files.

* Using statik as a code generator

* Updating statusz.go

* Updating statusz.go and main.go

* Updating statusz.go

* Adding dependcies from running go mod vendor

* Updating statik

Co-authored-by: Javier Kohen <jkohen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants