docs: Add documentation-test for run-command.md by mqasimsarfraz · Pull Request #52 · Azure/kubectl-aks

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

blanquicet

Fist set of comments after a quick review.

blanquicet · 2023-10-30T15:59:37Z

+## Testing
+
+### integration tests
+
+The integration test needs an AKS cluster to run against. You can create one and use following command to run the integration test:
+
+```
+make integration-test
+```
+
+You will need to set `AZURE_RESOURCE_GROUP` and `AZURE_CLUSTER_NAME` environment variables to specify the AKS cluster to run against.
+
+### documentation tests
+
+The documentation tests are used to validate the documentation examples. You can run the documentation tests with the following command:
+
+```
+make documentation-test
+```
+
+or
+
+```
+DOCUMENTATION_TEST_FILES=README.md make documentation-test -o install
+```
+
+An INI file with all the required variables needs to be created to run the documentation tests. A sample INI file for [docs/run-command.md](docs/run-command.md)
+is available [here](docs/run-command.ini.sample).
+


IMO, this should go in another file like "CONTRIBUTING.md" or so. I'd prefer to keep the README clean and simple.

blanquicet · 2023-10-30T16:02:06Z

+```
+make integration-test
+```


It would be useful if there is a way to avoid the InnovationEngine executes a code block instead of having to remove the bash.

blanquicet · 2023-10-30T17:23:20Z

+              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 define them as env variables in the next job step. WDYT?

- name: Run Documentation tests env: SUBSCRIPTION: ${{ secrets.AZURE_AKS_SUBSCRIPTION_ID }} ... run: make documentation-test -o install

I suggest to continue with the ini file approach.

blanquicet · 2023-10-30T17:31:50Z

 When executing `run-command` without passing through Kubernetes, we need to provide the information of the node (VMSS instance) where we want to run the given command. To retrieve such information, we can use the [`config import`](./config.md#importing-configuration) command. Once we got it, we can select the node we want to use and all the subsequent `run-command` commands will be executed on that node:
-
 ```bash
+# Clean the current configuration (if any)


I think comments should now become normal text and it should always looks like:

Now, let's do ... -bash commands- In a cluster with three nodes, the output will look like this: <!!--expected_similarity=X--> -output- Next, let's do that ... -bash commands- And this time the output ... <!!--expected_similarity=X--> -output-

Instead of a set of consecutive code blocks as it is now:

-bash commands- <!!--expected_similarity=X--> -output- -bash commands- <!!--expected_similarity=X--> -output- -bash commands- <!!--expected_similarity=X--> -output-

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

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

-        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

+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

+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

+```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

+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

  GO_VERSION: 1.18
  AZURE_PREFIX: kubectl-aks-ci
-  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


-```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

+              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


-```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

 ```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

+```
+
+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 October 30, 2023 10:25 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 10:29 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 10:34 — with GitHub Actions Inactive

mqasimsarfraz marked this pull request as ready for review October 30, 2023 10:48

mqasimsarfraz force-pushed the qasim/exec-docs branch 4 times, most recently from 6ee35b0 to b9abeeb Compare October 30, 2023 17:28

mqasimsarfraz force-pushed the qasim/exec-docs-run-command branch from f977fcc to 0646379 Compare October 30, 2023 17:30

blanquicet reviewed Oct 30, 2023

View reviewed changes

blanquicet mentioned this pull request Oct 30, 2023

docs: Add documentation-test for README #51

Merged

mqasimsarfraz temporarily deployed to aks October 30, 2023 17:50 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 17:55 — with GitHub Actions Inactive

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

mqasimsarfraz temporarily deployed to aks October 30, 2023 17:55 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 18:02 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs branch from b9abeeb to 8e2f76b Compare November 1, 2023 09:00

mqasimsarfraz changed the title ~~docs: Add test for run-command.md~~ docs: Add documentation-test for run-command.md Nov 1, 2023

mqasimsarfraz marked this pull request as draft November 1, 2023 11:08

mqasimsarfraz force-pushed the qasim/exec-docs branch 3 times, most recently from 9abf4d2 to cfad0e1 Compare November 10, 2023 14:23

docs: Add test for run-command.md

4dda796

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

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

		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
	AZURE_NODE_COUNT: 4 # multiple nodes are needed to allow running parallel 'run-command' against the same cluster. See create-aks-cluster.nodes job.

Uh oh!

Conversation

mqasimsarfraz commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blanquicet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mqasimsarfraz commented Oct 27, 2023 •

edited

Loading