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

fix: do not try and authenticate when error reporting is disabled #676

Merged
merged 6 commits into from
Oct 13, 2022
Merged

fix: do not try and authenticate when error reporting is disabled #676

merged 6 commits into from
Oct 13, 2022
+31 −4

Conversation

comp615
Copy link
Contributor

@comp615 comp615 commented Oct 3, 2022 •
edited
Loading

Fixes #550

@comp615 comp615 requested review from a team as code owners October 3, 2022 16:55
@google-cla
Copy link

google-cla bot commented Oct 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 3, 2022
@product-auto-label product-auto-label bot added the api: clouderrorreporting Issues related to the googleapis/nodejs-error-reporting API. label Oct 3, 2022
@losalex losalex assigned losalex and unassigned arbrown Oct 3, 2022
@losalex losalex self-requested a review October 3, 2022 17:10
@comp615
Copy link
Contributor Author

comp615 commented Oct 3, 2022

@losalex anything additional needed from me to get the tests going and eventual merge/release? I think because I'm first time I might need someone to kick those off for me

@losalex
Copy link
Contributor

losalex commented Oct 4, 2022

@losalex anything additional needed from me to get the tests going and eventual merge/release? I think because I'm first time I might need someone to kick those off for me

Thanks @comp615 for making this change! Added a comment - basically you need to add additional config parameter controlling this feature and add all relevant tests to make sure that this flag by default does not impacts existing code and also does not introduces new failures. Also - did you run this change locally and tested E2E with GCP?

losalex
losalex suggested changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

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

Please see my comments

src/google-apis/auth-client.ts Show resolved Hide resolved
@comp615 comp615 requested a review from losalex October 11, 2022 15:43
@losalex losalex added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2022
@comp615 comp615 force-pushed the ccroom/disable_auth_on_disable branch from eef1da0 to 44c42f2 Compare October 11, 2022 20:37
@losalex losalex added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 11, 2022
losalex
losalex approved these changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

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

LGTM - please run this change in GCP E2E with getShouldReportErrorsToAPI on/off to make sure it works as expected

@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 11, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 11, 2022
@comp615
Copy link
Contributor Author

comp615 commented Oct 11, 2022

LGTM - please run this change in GCP E2E with getShouldReportErrorsToAPI on/off to make sure it works as expected

We've been running this E2E since last week and have not observed any new errors on either side of the boolean. That value is off/false in our test/development environments and on in our production environments.

losalex
losalex reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@losalex losalex left a comment

Choose a reason for hiding this comment

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

Seems there are bunch of system tests failing - see https://source.cloud.google.com/results/invocations/b2b15578-b54f-4360-8620-bbc5fece441b/targets/cloud-devrel%2Fclient-libraries%2Fnodejs%2Fpresubmit%2Fgoogleapis%2Fnodejs-error-reporting%2Fnode12%2Fsystem-test/log

@comp615
Copy link
Contributor Author

comp615 commented Oct 12, 2022 •
edited
Loading

Try that new build. I'm really struggling to get the last few error-reporting tests running locally, so kind of guessing at what the tests need if anything. Would be much easier to debug in CI :-/

@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 12, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 12, 2022
@losalex losalex added the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 12, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Oct 12, 2022
@losalex losalex merged commit c9cef5a into googleapis:main Oct 13, 2022
@release-please release-please bot mentioned this pull request Oct 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@losalex losalex losalex approved these changes

Assignees

@losalex losalex

Labels
api: clouderrorreporting Issues related to the googleapis/nodejs-error-reporting API. size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Only try to authenticate if reporting is enabled.
4 participants
@comp615 @losalex @arbrown @yoshi-kokoro