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

Make ROLLBAR_TOKEN optional, provide PACKET_ var name alternatives | ENG-7652 #31

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

splaspood
Copy link
Contributor

@splaspood splaspood commented Jul 30, 2020

Since the 'tinkerbell' suite is no longer solely utilized by Packet/Equinix, it
stands to reason not every user/deployment will wish to integrate with rollbar
for exception logging. This change makes the rollbar integration optional.

Additionally we provide alternative env var names for PACKET_VERSION and PACKET_ENV (VERSION and ENV respectively). These env vars are only required if ROLLBAR_TOKEN is defined.

Copy link
Member

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

Typo in the commit comment:
s/not ever user/not every user/

log/log.go Outdated Show resolved Hide resolved
@splaspood splaspood force-pushed the ENG-7652 branch 2 times, most recently from d21c584 to b2d0da5 Compare July 30, 2020 16:34
@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #31 into master will increase coverage by 0.30%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   74.55%   74.85%   +0.30%     
==========================================
  Files           5        5              
  Lines         334      338       +4     
==========================================
+ Hits          249      253       +4     
  Misses         71       71              
  Partials       14       14              
Impacted Files Coverage Δ
log/internal/rollbar/rollbar.go 41.53% <0.00%> (ø)
log/log.go 80.95% <100.00%> (+1.29%) ⬆️

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 a2cb6bb...1bbb996. Read the comment docs.

@splaspood splaspood force-pushed the ENG-7652 branch 2 times, most recently from 3557e89 to ecbd39a Compare July 30, 2020 16:45
@splaspood splaspood changed the title Make ROLLBAR_TOKEN optional | ENG-7652 Make ROLLBAR_TOKEN optional, provide PACKET_ var name alternatives | ENG-7652 Jul 31, 2020
log/internal/rollbar/rollbar.go Outdated Show resolved Hide resolved
log/internal/rollbar/rollbar.go Outdated Show resolved Hide resolved
@splaspood splaspood force-pushed the ENG-7652 branch 4 times, most recently from b1e0f24 to d092caa Compare August 3, 2020 15:08
mmlb
mmlb previously approved these changes Aug 3, 2020
DavidHwu
DavidHwu previously approved these changes Aug 3, 2020
@DavidHwu
Copy link

DavidHwu commented Aug 3, 2020

Presumably the tests will be fixed or discussed before merge? Are these tests related?
@codecov
codecov/patch — 53.84% of diff hit (target 74.55%)
Details
@codecov
codecov/project — 74.85% (+0.30%) compared to 0602817

Since the 'tinkerbell' suite is no longer solely utilized by Packet/Equinix, it
stands to reason not every user/deployment will wish to integrate with rollbar
for exception logging.    This change makes the rollbar integration optional.

Additionally the example log function tests were moved to their own file.
@mmlb mmlb dismissed stale reviews from DavidHwu and themself via 1bbb996 September 3, 2020 15:50
@mmlb
Copy link
Member

mmlb commented Sep 3, 2020

Presumably the tests will be fixed or discussed before merge? Are these tests related?
@codecov
codecov/patch — 53.84% of diff hit (target 74.55%)
Details
@codecov
codecov/project — 74.85% (+0.30%) compared to 0602817

codecov isn't used as a blocking check, at the moment its used to see if coverage goes down and to try to figure out why.

@mmlb mmlb merged commit 0433e06 into master Sep 3, 2020
@mmlb mmlb deleted the ENG-7652 branch September 3, 2020 15:53
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.

3 participants