testserver: Modify GetHTTPClient to GetUnauthenticatedHTTPClient, GetAdminAuthenticatedHTTPClient to GetAdminHTTPClient by changhanlee · Pull Request #77732 · cockroachdb/cockroach

changhanlee · 2022-03-13T15:20:53Z

fixes #76241.
See commit message.

Release justification: Low impact, rename refactoring of testserver methods

cockroach-teamcity · 2022-03-13T15:20:55Z

All committers have signed the CLA.

blathers-crl · 2022-03-13T15:20:56Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2022-03-13T15:21:05Z

This change is

blathers-crl · 2022-03-13T15:52:10Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

knz

Reviewed 18 of 18 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @srosenberg)

knz · 2022-03-14T15:03:40Z

This change is good. Thank you!

knz · 2022-03-14T15:03:49Z

bors r+

craig · 2022-03-14T16:29:13Z

Build failed:

GitHub CI (Cockroach)

changhanlee · 2022-03-14T17:16:45Z

TFYR

otan · 2022-03-14T23:14:38Z

looks like there are merge conflicts - can you resolve them?

changhanlee · 2022-03-14T23:25:43Z

Yep, will do.

blathers-crl · 2022-03-15T03:21:59Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

changhanlee · 2022-03-15T07:04:48Z

@otan I fixed the merge conflict but now the TeamCity build fails. I looked through the build output logs multiple times but cannot understand the issue. I'd appreciate help in understanding the problem.

Also, when handling merge conflicts after approval, is it standard to rebase afterwards (as I have done here)?

otan · 2022-03-15T07:32:16Z

i see

[05:00:51][Run unit tests] pkg/ccl/serverccl/statusccl/tenant_test_utils.go:173:27: too many arguments in call to c.tenantHTTPClient
[05:00:51][Run unit tests] 	have (*testing.T, serverIdx, bool)
[05:00:51][Run unit tests] 	want (*testing.T, serverIdx)
[05:00:51][Run unit tests] pkg/ccl/serverccl/statusccl/tenant_status_test.go:889:50: too many arguments in call to helper.testCluster().tenantHTTPClient
[05:00:51][Run unit tests] 	have (*testing.T, number, bool)
[05:00:51][Run unit tests] 	want (*testing.T, serverIdx)
[05:00:51][Run unit tests] pkg/ccl/serverccl/statusccl/tenant_status_test.go:1022:50: too many arguments in call to helper.testCluster().tenantHTTPClient
[05:00:51][Run unit tests] 	have (*testing.T, number, bool)
[05:00:51][Run unit tests] 	want (*testing.T, serverIdx)
[05:00:51][Run unit tests] compilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/compile: exit status 2
[05:00:51][Run unit tests] INFO: Elapsed time: 1083.721s, Critical Path: 693.45s
[05:00:51][Run unit tests] INFO: 6711 processes: 733 internal, 5978 processwrapper-sandbox.

when clicking Build Log

Also, when handling merge conflicts after approval, is it standard to rebase afterwards (as I have done here)?

yep!

blathers-crl · 2022-03-15T11:34:49Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

…AdminAuthenticatedHTTPClient to GetAdminHTTPClient Release justification: Low impact, rename refactoring of testserver methods Release note (security upgrade): Modifies default testserver method of getting http unauthenticated clients with a more verbose name to encourage use of authenticated sessions. The default authenticated admin http client getter is simplified in turn.

knz · 2022-03-15T14:37:51Z

thank you for the change again. When the CI is happy with this, we can merge this

knz · 2022-03-15T15:48:30Z

bors r+

knz · 2022-03-15T22:34:49Z

bors r+

craig · 2022-03-16T00:27:45Z

Build succeeded:

GitHub CI (Cockroach)

changhanlee requested a review from a team March 13, 2022 15:20

changhanlee requested review from a team as code owners March 13, 2022 15:20

changhanlee requested review from a team and srosenberg and removed request for a team March 13, 2022 15:20

blathers-crl bot added the O-community Originated from the community label Mar 13, 2022

changhanlee force-pushed the tserver-unauth branch from 998db8b to 5c9578d Compare March 13, 2022 15:52

changhanlee force-pushed the tserver-unauth branch 2 times, most recently from 68baac2 to 4e8dfd5 Compare March 13, 2022 21:27

knz approved these changes Mar 14, 2022

View reviewed changes

matthewtodd mentioned this pull request Mar 14, 2022

server: precisely route cancel query/session requests for tenants #77550

Merged

changhanlee force-pushed the tserver-unauth branch from 4e8dfd5 to 71efc61 Compare March 15, 2022 01:27

changhanlee force-pushed the tserver-unauth branch from c307711 to 44f1b43 Compare March 15, 2022 04:37

changhanlee force-pushed the tserver-unauth branch from 44f1b43 to ba4e933 Compare March 15, 2022 10:35

changhanlee force-pushed the tserver-unauth branch from ba4e933 to d473771 Compare March 15, 2022 11:26

changhanlee force-pushed the tserver-unauth branch 2 times, most recently from d473771 to fa14dc1 Compare March 15, 2022 11:39

changhanlee force-pushed the tserver-unauth branch from fa14dc1 to e042fbc Compare March 15, 2022 12:52

craig bot merged commit be54b85 into cockroachdb:master Mar 16, 2022

cockroach-teamcity mentioned this pull request Mar 16, 2022

testserver: Modify GetHTTPClient to GetUnauthenticatedHTTPClient, GetAdminAuthenticatedHTTPClient to GetAdminHTTPClient cockroachdb/docs#13256

Closed

changhanlee deleted the tserver-unauth branch March 16, 2022 00:59

Conversation

changhanlee commented Mar 13, 2022 • edited by knz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Mar 13, 2022

Uh oh!

cockroach-teamcity commented Mar 13, 2022

Uh oh!

blathers-crl bot commented Mar 13, 2022

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

knz commented Mar 14, 2022

Uh oh!

knz commented Mar 14, 2022

Uh oh!

craig bot commented Mar 14, 2022

Uh oh!

changhanlee commented Mar 14, 2022

Uh oh!

otan commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changhanlee commented Mar 14, 2022

Uh oh!

blathers-crl bot commented Mar 15, 2022

Uh oh!

changhanlee commented Mar 15, 2022

Uh oh!

otan commented Mar 15, 2022

Uh oh!

blathers-crl bot commented Mar 15, 2022

Uh oh!

knz commented Mar 15, 2022

Uh oh!

knz commented Mar 15, 2022

Uh oh!

knz commented Mar 15, 2022

Uh oh!

craig bot commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changhanlee commented Mar 13, 2022 •

edited by knz

Loading

cockroach-teamcity commented Mar 13, 2022 •

edited

Loading

otan commented Mar 14, 2022 •

edited

Loading