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.

Sign up for GitHub

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

Jump to bottom

Expose list of configured exporters #16

Merged
merged 14 commits into from Apr 10, 2019
Merged

Expose list of configured exporters #16

merged 14 commits into from Apr 10, 2019

Conversation

ldaneliukas
Copy link
Contributor

PR for #15

image

@ldaneliukas ldaneliukas mentioned this pull request Oct 2, 2018
Closed
@tdabasinskas
Copy link

Hi, @tcolgate ,

Were you able to look into this PR? Just curious if this project is still alive 🙂

tcolgate
tcolgate reviewed Mar 28, 2019
View reviewed changes
main.go Show resolved Hide resolved
@tcolgate
Copy link
Collaborator

Hi,
very sorry I missed this. This looks like a great idea. I put a comment on the PR, basically, probably better to either escape the text with whatever html/template uses, or use html/template directly.

@ldaneliukas
Copy link
Contributor Author

Hey, @tcolgate - glad to see this is still maintained, we're quite heavy users of it :)

Is the PR good now?

tcolgate
tcolgate requested changes Mar 29, 2019
View reviewed changes
Copy link
Collaborator

@tcolgate tcolgate left a comment

Choose a reason for hiding this comment

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

Sorry to be picky:

  • Please rebase (hopefully the master's use of go mod wont cause you pain
  • Other comments in line.
    Should be good to merge once these are resolved.

README.md Outdated
@@ -101,3 +104,10 @@ scrape_configs:
static_configs:
- targets: ['host:9999']
```

## Building / releasing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Drop these, they aren't relevant to this PR, I've already tweaked in the README a bit, I'll add these, or equivalent in there directly.

main.go
<li><a href="/proxy?module={{$name}}">{{$name}}</a></li>
{{end}}
</ul>`)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should never fail, so I'm tempted to say you can just _ out the err above. Failing that, if we check the error here, then please http.Error(....) and return (you are currently exeuting the template even if this fails).

main.go Outdated
if err != nil {
log.Error(err)
}
tmpl.Execute(w, cfg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check the error from Execute.

@ldaneliukas
Copy link
Contributor Author

Update the PR. There's also quite a few of go.mod changes, due to go mod tidy, I hope that's okey.

@tcolgate
Copy link
Collaborator

The PR is showing conflicts, I'm just trying to rebase locally.

@ldaneliukas
Copy link
Contributor Author

Rebased the fork against upstream

@tcolgate
Copy link
Collaborator

Great, sorry that got a bit painful, the PR was trying to rip out a bunch of fixes.
Thanks, this is a really nice addition.

@tcolgate tcolgate merged commit e88e9da into QubitProducts:master Apr 10, 2019
@candlerb
Copy link
Contributor

Any chance of a new binary release? This change was merged shortly after v0.2.9 was released. Thanks!

@tcolgate
Copy link
Collaborator

tcolgate commented Oct 22, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcolgate tcolgate tcolgate approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ldaneliukas @tdabasinskas @tcolgate @candlerb