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.

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

examples/operator: Fix port-forward command. #6418

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

enisoc
Copy link
Member

@enisoc enisoc commented Jul 7, 2020

Naming the aliases the same as the underlying command has been tripping up users. Also fix port-forwarding for the possibility of
multiple query results, and send vtctl output to the console by default instead of log files.

cc @askdba

@enisoc enisoc requested a review from sougou July 7, 2020 20:42
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Let's wait a bit and see if there's other feedback.

@@ -1,13 +1,13 @@
#!/bin/sh

kubectl port-forward --address localhost deployment/"$(kubectl get deployment --selector="planetscale.com/component=vtctld" -o=jsonpath="{.items..metadata.name}")" 15000 15999 &
Copy link
Contributor

Choose a reason for hiding this comment

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

I pulled this from some planetscale documentation. We have to find out where that was and fix it there too.

@@ -9,47 +9,47 @@ kubectl apply -f operator.yaml

# Bring up initial cluster and commerce keyspace
kubectl apply -f 101_initial_cluster.yaml
vtctlclient ApplySchema -sql="$(cat create_commerce_schema.sql)" commerce
vtctlclient ApplyVSchema -vschema="$(cat vschema_commerce_initial.json)" commerce
kvtctl ApplySchema -sql="$(cat create_commerce_schema.sql)" commerce
Copy link
Contributor

Choose a reason for hiding this comment

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

@morgo wanted to use the original names. The intent was that we'll have a config directory like kubectl does. But we haven't done that part yet.

But I'm ok with using a different name until that part is done. This has confused me also a couple of times.

@sougou
Copy link
Contributor

sougou commented Jul 9, 2020

It just occurred to me that we have to fix the documentation to match this change. Will you have time for it?

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.

I looked at the website docs (Getting started guides). We have a unified flow for all the flavors (operator, local and helm), which means the same aliases have to be used by all 3 examples.
The other changes (pf, logging) are important. Can we keep just those and revert the alias changes?

@enisoc
Copy link
Member Author

enisoc commented Jul 21, 2020

We have a unified flow for all the flavors

Ah I didn't know that. Good catch.

The other changes (pf, logging) are important. Can we keep just those and revert the alias changes?

Yes I'll update the PR.

Previously, the command would fail if `kubectl get` returned multiple
entries, as would happen if there are multiple cells deployed.

Also send vtctl output to the console by default
instead of log files.

Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
@enisoc enisoc changed the title examples/operator: Change recommended port-forward shell aliases. examples/operator: Fix port-forward command. Jul 22, 2020
@enisoc enisoc requested a review from deepthi July 22, 2020 22:05
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.

@deepthi deepthi merged commit 1ab0652 into vitessio:master Jul 22, 2020
@enisoc enisoc deleted the operator-example-aliases branch July 23, 2020 01:03
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants