docs: Add documentation-test for run-command.md #52

mqasimsarfraz · 2023-10-27T16:22:59Z

Add test for run-command.md. In order to test it locally you need to write an INI file at docs/run-command.ini with sample:

→ cat docs/run-command.ini.sample 
mySubID = mySubID
myRG = myRG
myCluster = myCluster
myNode = myNode

Then use following to run the tests:

make documentation-test-commands

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

blanquicet · 2024-02-05T15:12:09Z

.github/workflows/kubectl-aks.yml

-        run: make documentation-test -o kubectl-aks
+        run: make documentation-test-readme


Why did you remove the -o kubectl-aks?

blanquicet · 2024-02-05T15:22:43Z

docs/run-command.md

+kubectl aks config unset-all
+```
+
+Import the nodes information with the cluster information:


Suggested change

Import the nodes information with the cluster information:

Import the nodes information with the cluster information:

> [!NOTE]

> Passing the cluster information to the `config import` command is optional. However, in this specific case, it will force the command to retrieve the nodes information from the Azure API server instead of the Kubernetes API server, making this approach more resilient to issues on the Kubernetes control plane.

blanquicet · 2024-02-05T15:27:19Z

docs/run-command.md

+In case we want to print the imported information, we can use the `show` command:
+
+```bash
+kubectl aks config show
+```


If you check, we always execute config show after a config import. I started thinking that we should automatically print the imported data in the config import (maybe with a --quiet flag). Also for the documentation, it makes things more consistent. For instance:

Import node info:

kubectl aks config import

Run command against one of the nodes (Folks will take the node name from the previous output):

kubectk aks run-command --node <myNode>

WDYT?

blanquicet · 2024-02-05T15:31:14Z

docs/run-command.md

+```bash
+kubectl aks run-command "ip route"
+```

-# Execute the run-command, and it will be automatically executed in aks-agentpool-12345678-vmss000000
-$ kubectl aks run-command "ip route"
+The output should be similar to this:
+
+<!--expected_similarity=0.8-->
+```


I'm wondering if we should use another command that provides a more deterministic output so that we can use  or some regex to match an expected string.

Of course, this is something we can do later, not for this PR.

blanquicet · 2024-02-05T15:34:38Z

docs/run-command.md

+Unset the current node to avoid conflict with flags or environment variables:
+
+```bash 
+kubectl aks config unset-current-node
+```


#55 should solve this issue. It shouldn't be necessary anymore. In the documentation we said that the precedence is flags, env variables, config file. So, using the flag --id should simply have the precedence and don't go in conflict with the config file.

blanquicet · 2024-02-05T15:43:03Z

.github/workflows/kubectl-aks.yml

-  AZURE_NODE_COUNT: 3 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster
+  AZURE_NODE_COUNT: 4 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster


Suggested change

AZURE_NODE_COUNT: 3 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster

AZURE_NODE_COUNT: 4 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster

AZURE_NODE_COUNT: 4 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster. See create-aks-cluster.nodes job.

blanquicet · 2024-02-05T15:44:23Z

docs/run-command.md

 On the other side, if we already have the node (VMSS instance) information and we don't want/need to save it locally, we could pass it directly as following:

-```bash
-kubectl aks run-command "ip route" --id "/subscriptions/$SUBSCRIPTION/resourceGroups/$NODERESOURCEGROUP/providers/Microsoft.Compute/virtualMachineScaleSets/$VMSS/virtualmachines/$INSTANCEID"
+<!-- TODO: Test following when we have a simple way to get instance information -->


We could do it with az in the CI and store that info in the ini file.

blanquicet · 2024-02-05T15:45:43Z

.github/workflows/kubectl-aks.yml

+              echo "mySubID = ${{ secrets.AZURE_AKS_SUBSCRIPTION_ID }}" >> $f
+              echo "myRG = ${{ env.AZURE_PREFIX }}-rg" >> $f
+              echo "myCluster = ${{ env.AZURE_PREFIX }}-amd64-cluster" >> $f
+              echo "myNode = $(echo '${{ needs.create-aks-cluster.outputs.documentation-nodes }}' | jq -r ".[0]")" >> $f


I suggest to continue with the ini file approach.

blanquicet · 2024-02-05T15:46:29Z

docs/run-command.md

 ```

-```bash
-kubectl aks run-command "ip route" --subscription $SUBSCRIPTION --node-resource-group $NODERESOURCEGROUP --vmss $VMSS --instance-id $INSTANCEID
+Or using the flags:


Suggested change

Or using the flags:

Or using the individual flags:

BTW, please add also the same TODO comment here.

blanquicet · 2024-02-05T15:48:18Z

docs/run-command.md

 ```bash
-kubectl aks run-command "ip route" --node aks-agentpool-12345678-vmss000000
+kubectl aks run-command "ip route" --node $myNode
 ```


After #52, we should need to clean up the config file. I think it is fine. We could add the unset-all here and explain that otherwise, the info already available in the config file will be re-used.

blanquicet · 2024-02-05T15:52:04Z

docs/run-command.md

+```
+
+Start using one of the nodes e.g `aks-agentpool-12345678-vmss000000` we call it `$myNode` here:
+
+```bash
+kubectl aks config use-node $myNode
+```

-# Start using one of the nodes
-kubectl aks use-node aks-agentpool-12345678-vmss000000
+Execute the run-command, and it will be automatically executed in `aks-agentpool-12345678-vmss000000`:

-# Execute the run-command, and it will be automatically executed in aks-agentpool-12345678-vmss000000
+```bash
 kubectl aks run-command "ip route"
+```
+
+If we run another command, it will again be executed in `aks-agentpool-12345678-vmss000000`:

-# Another command that will be still executed in aks-agentpool-12345678-vmss000000
+```bash
 kubectl aks run-command "hostname"
 ```


I don't know, it doesn't make sense to repeat everything here again. Probably, we should change the structure of this file:

The two mechanisms to retrieve the info (and we verify the output from both using ExecDocs).

Show the example (only once and no matter how we retrieve the info as we already tested that both are fine).

It's fine to manage this comment in later.

mqasimsarfraz temporarily deployed to aks October 27, 2023 16:25 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 42e65b7 to 841b46d Compare October 27, 2023 16:26

mqasimsarfraz temporarily deployed to aks October 27, 2023 16:28 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 16:33 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 16:40 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 841b46d to 81be06a Compare October 27, 2023 17:12

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:14 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:20 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:26 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 81be06a to 763d0d5 Compare October 27, 2023 17:45

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:48 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:53 — with GitHub Actions Inactive

mqasimsarfraz had a problem deploying to aks October 27, 2023 17:53 — with GitHub Actions Failure

mqasimsarfraz temporarily deployed to aks October 27, 2023 17:53 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 18:00 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 763d0d5 to aad8b88 Compare October 27, 2023 21:58

mqasimsarfraz temporarily deployed to aks October 27, 2023 22:01 — with GitHub Actions Inactive

mqasimsarfraz had a problem deploying to aks October 27, 2023 22:06 — with GitHub Actions Failure

mqasimsarfraz temporarily deployed to aks October 27, 2023 22:06 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from aad8b88 to c09ff6c Compare October 27, 2023 22:12

mqasimsarfraz temporarily deployed to aks October 27, 2023 22:18 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 22:22 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 27, 2023 22:23 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks December 7, 2023 12:29 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks December 7, 2023 12:35 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 717d4f9 to 26a81fc Compare December 7, 2023 12:37

mqasimsarfraz temporarily deployed to aks December 7, 2023 12:40 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 26a81fc to 6c87024 Compare December 7, 2023 12:40

mqasimsarfraz marked this pull request as ready for review December 7, 2023 12:41

mqasimsarfraz temporarily deployed to aks December 7, 2023 12:57 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks December 7, 2023 13:03 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks December 7, 2023 13:08 — with GitHub Actions Inactive

docs: Add test for run-command.md

4dda796

Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from 6c87024 to 4dda796 Compare January 8, 2024 15:15

mqasimsarfraz temporarily deployed to aks January 8, 2024 15:16 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks January 8, 2024 15:21 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks January 8, 2024 15:26 — with GitHub Actions Inactive

blanquicet added this to the v0.3.0 milestone Feb 2, 2024

blanquicet self-requested a review February 5, 2024 14:56

blanquicet modified the milestones: v0.3.0, 0.2.4 Feb 5, 2024

blanquicet reviewed Feb 5, 2024

View reviewed changes

blanquicet mentioned this pull request Feb 5, 2024

Print imported information after running config import #60

Open

blanquicet modified the milestones: 0.2.4, v0.2.5 Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Add documentation-test for run-command.md #52

docs: Add documentation-test for run-command.md #52

mqasimsarfraz commented Oct 27, 2023 •

edited

Loading

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

blanquicet Feb 5, 2024

		run: make documentation-test -o kubectl-aks
		run: make documentation-test-readme

		AZURE_NODE_COUNT: 3 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster
		AZURE_NODE_COUNT: 4 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster

docs: Add documentation-test for run-command.md #52

Are you sure you want to change the base?

docs: Add documentation-test for run-command.md #52

Conversation

mqasimsarfraz commented Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mqasimsarfraz commented Oct 27, 2023 •

edited

Loading