docs: Add documentation-test for README by mqasimsarfraz · Pull Request #51 · Azure/kubectl-aks

mqasimsarfraz · 2023-10-27T12:43:20Z

This change add support for testing documentation using exec-docs

blanquicet

Looking great already!

blanquicet · 2023-10-30T12:32:29Z

+      - name: Get kubectl-aks from artifact
+        uses: actions/download-artifact@v3
+        with:
+          name: kubectl-aks-linux-amd64-tar-gz
+      - name: Prepare kubectl-aks binary
+        shell: bash
+        run: |
+          tar zxvf kubectl-aks-linux-amd64.tar.gz
+          chmod +x kubectl-aks
+          mv kubectl-aks /usr/local/bin
+          ls -la


Why do you need to install it if it is going to be installed in the README?

I wanted to ensure that the binary that was built in the previous steps of the CI is actually tested since we eventually ship that in the release. But I need to ensure we clean up all other installation for it.

Yep but currently there is no way to test the README against the just built binary. See my comment in #52 (comment), I think we should separate the README from others docs and the README could only be run for the releases (which makes sense as it describes how to install a released version). WDYT?

currently there is no way to test the README against the just built binary

I realized it is possible to pass env vars to ie so it can be used to add custom directory to PATH like I did here. This takes care of using the built binary by adding path of the current directory where kubectl-aks (built by kubectl-aks target) is located. In the CI, kubectl-aks is download at correct location and used by the tests. Shouldn't that cover all the cases?

Edit: Need to ensure binaries at different path are used in expected order 🤔

I don't think it covers also the README case as it doesn't make sense to run kubectl krew install aks (which install kubectl-aks in ~/.krew/bin/ and then run kubectl aks version using the binary we compiled in the build CI job.

Then, for the commands documentation, we could use such method. However, I expect to have just one binary at that time given that we are cleaning up everything we install in the README. So, the only binary available in that host will be the one we downloaded from the artifacts.

I don't think it covers also the README case as it doesn't make sense to run kubectl krew install aks (which install kubectl-aks in ~/.krew/bin/ and then run kubectl aks version using the binary we compiled in the build CI job.

Thanks for catching this. I defined the precedence now krew > local > current directory and it is working as expected krew version, specific version, installed from source. This also ensures the kubectl aks --help is tested against the binary from the PR so any issues will be caught right away.

So, the only binary available in that host will be the one we downloaded from the artifacts.

Yeah, I think for commands testing, it is much simpler. I feel like with above we should be fine for README.md but I am happy to test it separately. What do you say?

I still find it too hacky. Now that you added the cleanup step for all the installation methods, I don't see the need of passing the list of paths in a specific order, there will always be only one binary installed at a time.

The only remaining problem is the kubectl aks help.... Mmmm I'm not sure what to do. We could either use ./kubectl-aks help or remove it as the list of commands are already describe above in the documentation.

What do you think?

blanquicet · 2023-10-30T13:02:49Z

 *.exe~
 *.dll
 *.so
+*.log


Is this file generated by the ie? I haven't noticed it.

Yeah, for me. I am getting ie.log once running it. Maybe it is only written if something fails?

I don't know. Please keep track of this things so that we can provide feedback to the InnovationEngine project.

blanquicet · 2023-11-09T13:00:31Z

+      - name: Get kubectl-aks from artifact
+        uses: actions/download-artifact@v3
+        with:
+          name: kubectl-aks-linux-amd64-tar-gz
+      - name: Prepare kubectl-aks binary
+        shell: bash
+        run: |
+          tar zxvf kubectl-aks-linux-amd64.tar.gz
+          chmod +x kubectl-aks
+          mv kubectl-aks /usr/local/bin
+          ls -la


I still find it too hacky. Now that you added the cleanup step for all the installation methods, I don't see the need of passing the list of paths in a specific order, there will always be only one binary installed at a time.

The only remaining problem is the kubectl aks help.... Mmmm I'm not sure what to do. We could either use ./kubectl-aks help or remove it as the list of commands are already describe above in the documentation.

What do you think?

mqasimsarfraz · 2023-11-10T13:47:39Z

The only remaining problem is the kubectl aks help.... Mmmm I'm not sure what to do. We could either use ./kubectl-aks help or remove it as the list of commands are already describe above in the documentation.

I agree. It does look hacky. I removed testing it for now. We can reflect back later on if there is a nicer way to do it. Thanks for all the feedback. I updated the PR.

blanquicet

LGTM! Thanks for working on this.

This change add support for testing documentation using exec-docs Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

mqasimsarfraz · 2023-11-10T14:42:32Z

Thanks for reviews! CI is green! I will merge this!

mqasimsarfraz force-pushed the qasim/exec-docs branch from df163ab to 94ab354 Compare October 27, 2023 12:45

mqasimsarfraz had a problem deploying to aks October 27, 2023 12:49 — with GitHub Actions Error

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

mqasimsarfraz force-pushed the qasim/exec-docs branch from 94ab354 to 5c28b06 Compare October 27, 2023 12:56

mqasimsarfraz had a problem deploying to aks October 27, 2023 13:01 — with GitHub Actions Error

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

mqasimsarfraz force-pushed the qasim/exec-docs branch from 5c28b06 to b342f22 Compare October 27, 2023 13:08

mqasimsarfraz had a problem deploying to aks October 27, 2023 13:11 — with GitHub Actions Error

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

mqasimsarfraz force-pushed the qasim/exec-docs branch from b342f22 to dde01de Compare October 27, 2023 13:16

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

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

mqasimsarfraz force-pushed the qasim/exec-docs branch 2 times, most recently from 46b9131 to ef7febc Compare October 27, 2023 13:31

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

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

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

mqasimsarfraz force-pushed the qasim/exec-docs branch from ef7febc to 2ed96ce Compare October 30, 2023 09:34

mqasimsarfraz changed the title ~~docs: Add test-docs~~ docs: Add documentation-test Oct 30, 2023

mqasimsarfraz temporarily deployed to aks October 30, 2023 09:36 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 09:41 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks October 30, 2023 09:48 — with GitHub Actions Inactive

blanquicet reviewed Oct 30, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/parallel-tests branch from 9107ee0 to 57ba94f Compare October 30, 2023 14:51

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

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

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

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

Base automatically changed from qasim/parallel-tests to main October 30, 2023 17:33

blanquicet changed the title ~~docs: Add documentation-test~~ docs: Add documentation-test for README Oct 30, 2023

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

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

blanquicet reviewed Oct 31, 2023

View reviewed changes

Comment thread .github/workflows/kubectl-aks.yml Outdated

Comment thread .github/workflows/kubectl-aks.yml

Comment thread Makefile Outdated

Comment thread README.md Outdated

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

mqasimsarfraz temporarily deployed to aks November 1, 2023 09:02 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks November 1, 2023 09:08 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks November 1, 2023 09:13 — with GitHub Actions Inactive

blanquicet reviewed Nov 9, 2023

View reviewed changes

mqasimsarfraz force-pushed the qasim/exec-docs branch from 8e2f76b to 2cc47ce Compare November 10, 2023 13:44

mqasimsarfraz temporarily deployed to aks November 10, 2023 13:47 — with GitHub Actions Inactive

mqasimsarfraz temporarily deployed to aks November 10, 2023 13:51 — with GitHub Actions Inactive

blanquicet approved these changes Nov 10, 2023

View reviewed changes

Comment thread README.md

mqasimsarfraz temporarily deployed to aks November 10, 2023 13:59 — with GitHub Actions Inactive

mqasimsarfraz force-pushed the qasim/exec-docs branch from 2cc47ce to 9abf4d2 Compare November 10, 2023 14:12

mqasimsarfraz had a problem deploying to aks November 10, 2023 14:14 — with GitHub Actions Error

docs: Add documentation-test

cfad0e1

This change add support for testing documentation using exec-docs Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>

Uh oh!

Conversation

mqasimsarfraz commented Oct 27, 2023

Uh oh!

blanquicet left a comment

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mqasimsarfraz commented Nov 10, 2023

Uh oh!

blanquicet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mqasimsarfraz commented Nov 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mqasimsarfraz Oct 30, 2023 •

edited

Loading

blanquicet Oct 31, 2023 •

edited

Loading