-
Notifications
You must be signed in to change notification settings - Fork 8
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
docs: Add documentation-test for README #51
Conversation
df163ab
to
94ab354
Compare
94ab354
to
5c28b06
Compare
5c28b06
to
b342f22
Compare
b342f22
to
dde01de
Compare
46b9131
to
ef7febc
Compare
ef7febc
to
2ed96ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great already!
.github/workflows/kubectl-aks.yml
Outdated
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to install it if it is going to be installed in the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
.gitignore
Outdated
@@ -3,6 +3,7 @@ | |||
*.exe~ | |||
*.dll | |||
*.so | |||
*.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file generated by the ie
? I haven't noticed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for me. I am getting ie.log once running it. Maybe it is only written if something fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. Please keep track of this things so that we can provide feedback to the InnovationEngine project.
9107ee0
to
57ba94f
Compare
b9abeeb
to
8e2f76b
Compare
.github/workflows/kubectl-aks.yml
Outdated
- 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
8e2f76b
to
2cc47ce
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for working on this.
2cc47ce
to
9abf4d2
Compare
This change add support for testing documentation using exec-docs Signed-off-by: Qasim Sarfraz <qasimsarfraz@microsoft.com>
9abf4d2
to
cfad0e1
Compare
Thanks for reviews! CI is green! I will merge this! |
This change add support for testing documentation using exec-docs