Refine logging in pkg/dashboard/apiserver/auth/gcp #3527

yoonian · 2022-08-04T12:58:12Z

What problem does this PR solve?

Close #2873

What's changed and how it works?

Moving package level variable log into struct Service as a field

Related changes

Checklist

CHANGELOG

I have updated the CHANGELOG.md
I have labeled this PR with "no-need-update-changelog"

Tests

Unit test
E2E test
No code
Manual test (add steps below)

Side effects

Breaking backward compatibility

ti-chi-bot · 2022-08-04T12:58:14Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Andrewmatilde
xlgao-zju

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2022-08-04T12:58:20Z

Welcome @yoonian!

It looks like this is your first PR to chaos-mesh/chaos-mesh 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to chaos-mesh/chaos-mesh. 😃

ti-chi-bot · 2022-08-04T12:58:36Z

@yoonian: The label(s) no-need-update-changelog cannot be applied. These labels are supported: bugbash, contribution, DNM, Hacktoberfest, dependencies, documentation, duplicate, good first issue, invalid, need-more-info, needs-cherry-pick-0.8, needs-cherry-pick-0.9, needs-cherry-pick-1.0, needs-cherry-pick-1.1, needs-cherry-pick-1.2, needs-cherry-pick-2.0, needs-cherry-pick-2.1, needs-cherry-pick-2.2, needs-cherry-pick-2.3, require-LGT3.

In response to this:

/label no-need-update-changelog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-08-04T12:58:52Z

@yoonian: The label(s) no-need-update-changelog cannot be applied. These labels are supported: bugbash, contribution, DNM, Hacktoberfest, dependencies, documentation, duplicate, good first issue, invalid, need-more-info, needs-cherry-pick-0.8, needs-cherry-pick-0.9, needs-cherry-pick-1.0, needs-cherry-pick-1.1, needs-cherry-pick-1.2, needs-cherry-pick-2.0, needs-cherry-pick-2.1, needs-cherry-pick-2.2, needs-cherry-pick-2.3, require-LGT3.

In response to this:

/label no-need-update-changelog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

STRRL

rest LGTM

STRRL · 2022-08-07T02:47:40Z

pkg/dashboard/apiserver/auth/gcp/fx.go

+func Bootstrap(conf *config.ChaosDashboardConfig, logger logr.Logger) *Service {
+	service, _ := NewService(conf, logger.WithName("gcp auth api"))
+	return service
+}


please do not ignore that error returned by NewService. After that, Bootstrap do not do extra things other than NewService, please use NewService with fx directly.

Thanks, I'll apply.

STRRL · 2022-08-07T02:47:59Z

pkg/dashboard/apiserver/auth/gcp/service.go

 ) (*Service, error) {
 	rootUrl, err := url.Parse(conf.RootUrl)
 	if err != nil {
+		logger.Error(err, "fail to parse rootUrl")


as we have returned the error, do not log it again.

codecov · 2022-08-07T02:49:55Z

Codecov Report

Merging #3527 (e3234e8) into master (8358017) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3527      +/-   ##
==========================================
+ Coverage   40.12%   40.18%   +0.06%     
==========================================
  Files         166      166              
  Lines       14187    14187              
==========================================
+ Hits         5692     5701       +9     
+ Misses       8062     8055       -7     
+ Partials      433      431       -2

Impacted Files	Coverage Δ
pkg/workflow/controllers/utils.go	`86.04% <0.00%> (-1.56%)`	⬇️
pkg/workflow/controllers/chaos_node_reconciler.go	`61.31% <0.00%> (+3.15%)`	⬆️

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 8358017...e3234e8. Read the comment docs.

Andrewmatilde

LGTM

xlgao-zju

LGTM

xlgao-zju · 2022-08-18T03:33:21Z

/merge

ti-chi-bot · 2022-08-18T03:33:23Z

This pull request has been accepted and is ready to merge.

Commit hash: 7ffe4d6

Andrewmatilde · 2022-08-19T07:15:57Z

/merge

ti-chi-bot · 2022-08-19T07:15:59Z

This pull request has been accepted and is ready to merge.

Commit hash: 5543d9d

ti-chi-bot · 2022-08-19T09:57:04Z

@yoonian: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Signed-off-by: Yoon PyungHo <learder@gmail.com>

…auth/gcp

Andrewmatilde · 2022-08-23T04:24:24Z

/merge

ti-chi-bot · 2022-08-23T04:24:27Z

This pull request has been accepted and is ready to merge.

Commit hash: e3234e8

ti-chi-bot added contribution first-time-contributor labels Aug 4, 2022

ti-chi-bot requested review from AsterNighT and iguoyr August 4, 2022 12:58

ti-chi-bot added the size/M label Aug 4, 2022

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from 80f13ab to 47459da Compare August 4, 2022 13:25

STRRL reviewed Aug 7, 2022

View reviewed changes

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from ca888bc to e1aaf36 Compare August 7, 2022 04:15

ti-chi-bot added size/S and removed size/M labels Aug 7, 2022

yoonian requested a review from STRRL August 9, 2022 16:03

Andrewmatilde approved these changes Aug 11, 2022

View reviewed changes

ti-chi-bot added status/LGT1 needs-rebase labels Aug 11, 2022

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from e1aaf36 to 7ffe4d6 Compare August 11, 2022 15:31

ti-chi-bot removed the needs-rebase label Aug 11, 2022

xlgao-zju approved these changes Aug 18, 2022

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Aug 18, 2022

ti-chi-bot added the status/can-merge label Aug 18, 2022

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from 5bdaa4f to 7ffe4d6 Compare August 18, 2022 08:57

ti-chi-bot added the needs-rebase label Aug 18, 2022

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from 2430b73 to 5543d9d Compare August 19, 2022 00:19

ti-chi-bot removed status/can-merge needs-rebase labels Aug 19, 2022

ti-chi-bot added the status/can-merge label Aug 19, 2022

chore: refine logging in pkg/dashboard/apiserver/auth/gcp

b709220

Signed-off-by: Yoon PyungHo <learder@gmail.com>

yoonian force-pushed the refine-logging-in-pkg/dashboard/apiserver/auth/gcp branch from 46bdcde to b709220 Compare August 19, 2022 10:07

ti-chi-bot removed the status/can-merge label Aug 19, 2022

Merge branch 'master' into refine-logging-in-pkg/dashboard/apiserver/…

e3234e8

…auth/gcp

ti-chi-bot added the status/can-merge label Aug 23, 2022

ti-chi-bot merged commit cbead3e into chaos-mesh:master Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine logging in pkg/dashboard/apiserver/auth/gcp #3527

Refine logging in pkg/dashboard/apiserver/auth/gcp #3527

yoonian commented Aug 4, 2022 •

edited by STRRL

Loading

ti-chi-bot commented Aug 4, 2022 •

edited

Loading

ti-chi-bot commented Aug 4, 2022

ti-chi-bot commented Aug 4, 2022

ti-chi-bot commented Aug 4, 2022

STRRL left a comment

STRRL Aug 7, 2022

yoonian Aug 7, 2022

STRRL Aug 7, 2022

yoonian Aug 7, 2022

codecov bot commented Aug 7, 2022 •

edited

Loading

Andrewmatilde left a comment

xlgao-zju left a comment

xlgao-zju commented Aug 18, 2022

ti-chi-bot commented Aug 18, 2022

Andrewmatilde commented Aug 19, 2022

ti-chi-bot commented Aug 19, 2022

ti-chi-bot commented Aug 19, 2022

Andrewmatilde commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

Refine logging in pkg/dashboard/apiserver/auth/gcp #3527

Refine logging in pkg/dashboard/apiserver/auth/gcp #3527

Conversation

yoonian commented Aug 4, 2022 • edited by STRRL Loading

What problem does this PR solve?

What's changed and how it works?

Related changes

Checklist

ti-chi-bot commented Aug 4, 2022 • edited Loading

ti-chi-bot commented Aug 4, 2022

ti-chi-bot commented Aug 4, 2022

ti-chi-bot commented Aug 4, 2022

STRRL left a comment

Choose a reason for hiding this comment

STRRL Aug 7, 2022

Choose a reason for hiding this comment

yoonian Aug 7, 2022

Choose a reason for hiding this comment

STRRL Aug 7, 2022

Choose a reason for hiding this comment

yoonian Aug 7, 2022

Choose a reason for hiding this comment

codecov bot commented Aug 7, 2022 • edited Loading

Codecov Report

Andrewmatilde left a comment

Choose a reason for hiding this comment

xlgao-zju left a comment

Choose a reason for hiding this comment

xlgao-zju commented Aug 18, 2022

ti-chi-bot commented Aug 18, 2022

Andrewmatilde commented Aug 19, 2022

ti-chi-bot commented Aug 19, 2022

ti-chi-bot commented Aug 19, 2022

Andrewmatilde commented Aug 23, 2022

ti-chi-bot commented Aug 23, 2022

yoonian commented Aug 4, 2022 •

edited by STRRL

Loading

ti-chi-bot commented Aug 4, 2022 •

edited

Loading

codecov bot commented Aug 7, 2022 •

edited

Loading