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

Add VtctldServer to vtcombo #7896

Merged
merged 3 commits into from
Apr 23, 2021
Merged

Add VtctldServer to vtcombo #7896

merged 3 commits into from
Apr 23, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Apr 19, 2021

Signed-off-by: Andrew Mason amason@slack-corp.com

Description

Testing

  1. build and run vttestserver: make build; ./bin/vttestserver
  2. get gRPC port: sudo lsof -iTCP -P -sTCP:LISTEN | grep vtcombo | awk '{print $9}' | sort | head -n 2 | head -n 1 | sed 's/.*://'
  3. run client:
❯ ./bin/vtctldclient --server "localhost:12592" GetKeyspaces
[
  {
    "name": "test_keyspace",
    "keyspace": {}
  }
]

Related Issue(s)

Checklist

  • Should this PR be backported? no
  • Tests were added or are not required n/a
  • Documentation was added or is not required n/a

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@ajm188 ajm188 requested review from deepthi and acharis April 19, 2021 20:28
@ajm188
Add VtctldServer to vtcombo
c85c7da 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
deepthi
deepthi requested changes Apr 19, 2021
View reviewed changes
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM, but needs a unit test.

@ajm188
Add unit test for VtcomboArguments()
db24245 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ghost
Copy link

ghost commented Apr 20, 2021

thanks, man

deepthi
deepthi approved these changes Apr 20, 2021
View reviewed changes
go/vt/vttest/environment_test.go Outdated
args := env.VtcomboArguments()

t.Run("service_map flag", func(t *testing.T) {
require.Contains(t, args, "-service_map", "vttest.LocalTestEnv must contain provide `-service_map` flag to vtcombo")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'm guessing this was intended to be
must provide ...
not
must contain provide ...

@ajm188
Fix assertion string
0ee5f73 
Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 ajm188 merged commit 482a700 into vitessio:master Apr 23, 2021
@ajm188 ajm188 deleted the am_vttest_vtctldserver branch April 23, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepthi deepthi deepthi approved these changes

@acharis acharis Awaiting requested review from acharis

Assignees
No one assigned
Labels
Component: Cluster management Type: Feature Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add grpc-vtctld to vttest/vtcombo service map
3 participants
@ajm188 @deepthi @systay