refactor: clean up types and imports by JustinBeckwith · Pull Request #409 · googleapis/nodejs-logging

JustinBeckwith · 2019-03-01T23:14:29Z

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)

Tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

codecov · 2019-03-01T23:44:20Z

Codecov Report

Merging #409 into master will decrease coverage by 0.04%.
The diff coverage is 86%.

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
- Coverage   90.49%   90.44%   -0.05%     
==========================================
  Files          14       14              
  Lines         631      628       -3     
  Branches       34       34              
==========================================
- Hits          571      568       -3     
  Misses         42       42              
  Partials       18       18

Impacted Files	Coverage Δ
src/sink.ts	`96.42% <ø> (ø)`	⬆️
src/index.ts	`99.44% <100%> (-0.01%)`	⬇️
src/entry.ts	`100% <100%> (ø)`	⬆️
src/log.ts	`80% <82.92%> (-0.24%)`	⬇️

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 9a2884e...9a19288. Read the comment docs.

jkwlui · 2019-03-04T22:40:50Z

@stephenplusplus are you able to look into why system tests are failing here?

stephenplusplus · 2019-03-06T19:50:33Z

I can't reproduce locally. Let's try a rebuild!

stephenplusplus · 2019-03-06T22:20:44Z

Rebuild still failed.

@jkwlui -- the error message, "Error: 3 INVALID_ARGUMENT: A monitored resource must be specified for each log entry", could relate to the logic where we find the most appropriate resource to go alongside a log entry dependent on the environment the code is in. That being said, I'm not sure how to simulate the Kokoro environment. Any ideas?

JustinBeckwith · 2019-03-06T22:26:27Z

Can you try something? If the grpc-js env var is set, just process.exit? I wonder if this is a race condition with system tests.

googlebot · 2019-03-06T22:36:59Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

stephenplusplus · 2019-03-06T22:42:52Z

I'm not 100% on what that env var is, but I went with this: 53691a1

Looks like Kokoro is having issues, though, so the suspense continues!

JustinBeckwith · 2019-03-06T22:49:11Z

@jkwlui is on the other issues

googlebot · 2019-03-07T02:49:16Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

ofrobots · 2019-03-07T04:27:45Z

      callback?: CreateSinkCallback): Promise<[Sink, LogSink]>|void {
    const self = this;
-    if (!is.string(name)) {
+    if (typeof name !== 'string') {


Would this be considered semver major?

[rant] Removal of is as a dependency doesn't fit into the spirit of this PR anyway. This only makes the review process more complex. Refactor PRs should be easy to review.

Open a separate PR for this change.
[/rant]

Happy to :) So question tho - would you prefer a total drop of is, a move to the other is, or keeping this as is 🥁

No objections to dropping is as semver-major. If the argument is that it is not breaking, it needs to be made.

googlebot · 2019-03-08T16:51:59Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 1, 2019

JustinBeckwith requested review from jkwlui and ofrobots March 1, 2019 23:16

jkwlui approved these changes Mar 2, 2019

View reviewed changes

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2019

stephenplusplus self-assigned this Mar 6, 2019

stephenplusplus added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Mar 6, 2019

JustinBeckwith added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 7, 2019

JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 7, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 7, 2019

ofrobots reviewed Mar 7, 2019

View reviewed changes

refactor: clean up types and imports

44e972d

JustinBeckwith unassigned stephenplusplus Mar 8, 2019

JustinBeckwith force-pushed the fixy branch from 22164d3 to 44e972d Compare March 8, 2019 16:51

yoshi-automation added the 🚨 This issue needs some love. label Mar 8, 2019

fixy

9a19288

JustinBeckwith merged commit 278696d into master Mar 9, 2019

stephenplusplus deleted the fixy branch February 13, 2020 14:14

snyk-bot mentioned this pull request Mar 1, 2020

[Snyk] Upgrade @google-cloud/logging from 4.1.1 to 4.5.2 wilddog64/spinnaker-for-gcp#1

Open

yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020

Conversation

JustinBeckwith commented Mar 1, 2019

Uh oh!

codecov Bot commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkwlui commented Mar 4, 2019

Uh oh!

stephenplusplus commented Mar 6, 2019

Uh oh!

stephenplusplus commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinBeckwith commented Mar 6, 2019

Uh oh!

googlebot commented Mar 6, 2019

Uh oh!

stephenplusplus commented Mar 6, 2019

Uh oh!

JustinBeckwith commented Mar 6, 2019

Uh oh!

googlebot commented Mar 7, 2019

Uh oh!

ofrobots Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

JustinBeckwith Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

ofrobots Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov Bot commented Mar 1, 2019 •

edited

Loading

stephenplusplus commented Mar 6, 2019 •

edited

Loading