Conversation
rgeyer
changed the title
|
This is ready for review, but before we cut a release, I'd like to get two other exporters sorted out. |
rfratto
left a comment
rfratto
left a comment
There was a problem hiding this comment.
LGTM! Just a few small pieces of feedback.
| os.Unsetenv("API_URL") | ||
| os.Unsetenv("REPOS") | ||
| os.Unsetenv("ORGS") | ||
| os.Unsetenv("USERS") | ||
| os.Unsetenv("GITHUB_TOKEN") | ||
| os.Unsetenv("GITHUB_TOKEN_FILE") |
There was a problem hiding this comment.
Hmm, we might want to consider restoring them to their old value (if it existed) and unsetting if it didn't.
There was a problem hiding this comment.
Ahh yes, will do, that's a bit less invasive.
| // It's not very pretty, but this exporter is configured entirely by environment | ||
| // variables, and uses some private helper methods in it's config package to | ||
| // assemble other key pieces of the config. Thus, we can't (easily) access the | ||
| // config directly, and must assign environment variables. | ||
| // | ||
| // In an effort to avoid conflicts with other integrations, the environment vars | ||
| // are unset immediately after being consumed. |
There was a problem hiding this comment.
😞 Can we open an issue upstream to address this? I'm not really a fan of needing to do this, but I'm comfortable getting this merged for now. Let's just stay on top of it and try to improve the situation.
There was a problem hiding this comment.
Yeah, as you can tell by the comment, I'm not a tremendous fan either.
I originally made this design choice when I thought I was going to use the exporter un-modified, but I have since had to make some small changes to a fork of the exporter anyhow.
I'll see if I can get this sorted and put in as a PR upstream as well.
There was a problem hiding this comment.
Did do..
Probably made it impossible for all my changes to get upstream, but..
We'll see 😂
mattdurham
left a comment
mattdurham
left a comment
There was a problem hiding this comment.
One change to documentation.
|
I'm not sure I understand why this PR conflicts with the CHANGELOG.md in main.. I thought I resolved them, but, this does not appear to be the case 🤔 |
rgeyer
force-pushed
the
rgeyer/github-integration
branch
from
fba59f4 to
4a9aa88
Compare
…mall bug fixes and labeling improvements. gofmt
… you is the wiser course.
…pendency to fork which cleans up the config steps
Co-authored-by: Robert Fratto <robert.fratto@grafana.com>
Co-authored-by: Robert Fratto <robert.fratto@grafana.com>
rgeyer
force-pushed
the
rgeyer/github-integration
branch
from
8eb832c to
66bf65b
Compare
* Initial commit of github exporter integration * Add github exporter config docs. Use a fork of github-exporter with small bug fixes and labeling improvements. gofmt * Linting fixes for github-exporter * Okay... More linting then * Maybe running the linter locally instead of waiting for CI to bark at you is the wiser course. * Minor github-exporter documentation change. Update github-exporter dependency to fork which cleans up the config steps * Update docs/configuration/integrations/github-exporter-config.md Co-authored-by: Robert Fratto <robert.fratto@grafana.com> * Update docs/configuration/integrations/github-exporter-config.md Co-authored-by: Robert Fratto <robert.fratto@grafana.com> * Check for errors from github exporter config * Actually import the packages you're using * Rebase to main and resolve go.sum conflict Co-authored-by: Robert Fratto <robert.fratto@grafana.com>
github-actions
Bot
added
the
frozen-due-to-age
3 participants
PR Description
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist