-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add K8s Authenticator Quickstart #326
Conversation
0085eb6
to
8e79165
Compare
@john-odonnell This looks good! This should make it easy to create a Katacoda scenario at some point! Maybe you can add a note in the Description that the QuickStart will improve when this Issue has been addressed (i.e. we can eliminate the local build of pet store): Another consideration is that in some places, it might be nice to have a sample output, or maybe a snippet of output to look for. (We've been keeping output code blocks separate from the commands to avoid having to add prompts and thereby making it awkward to cut/paste the commands.) Not that we need to go crazy with output for every command, just wherever it is helpful or can boost user confidence. |
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 good. Left some preemptive comments
bin/test-workflow/QUICKSTART.md
Outdated
This step wraps a Conjur OSS kind deployment | ||
[example](https://github.com/cyberark/conjur-oss-helm-chart/tree/main/examples/kubernetes-in-docker) | ||
found there. |
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 think it might be useful to rephrase this to provide more detail about the example folder and call out the scripts.
This step wraps a Conjur OSS kind deployment | |
[example](https://github.com/cyberark/conjur-oss-helm-chart/tree/main/examples/kubernetes-in-docker) | |
found there. | |
The Conjur OSS Helm Chart repository contains an [example](https://github.com/cyberark/conjur-oss-helm-chart/tree/main/examples/kubernetes-in-docker) folder with scripts and instructions for deploying the Conjur OSS Helm Chart on KinD. The scripts from the example folder are used to accomplish this step by git cloning the Conjur OSS Helm Chart repository and using them to carry out the tasks mentioned above. |
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 think we should also explicitly mention the need for git, can't be too careful.
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.
- Added detail to mention of Conjur OSS Helm Chart example
- Added
git
to list of prerequisites
bin/test-workflow/QUICKSTART.md
Outdated
|
||
At the end of this quickstart, you will have: | ||
|
||
* Deployed Conjur OSS to a kind cluster |
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.
This is purely an observation comment and you don't need to act on it other than potentially creating an issue for future work.
It's definitely simpler at this point to be prescriptive in this guide and mention only using the KinD cluster. However, in future work it would be nice if the KinD step became optional so that users could bring their own Kubernetes cluster. The requirement being that kubectl
is already setup to communicate to their cluster of choice. This would change the steps of the quickstart from:
- Deployed Conjur OSS to a kind cluster
- ...
to:
- Created a KinD cluster (optional)
- Deployed Conjur OSS to a Kubernetes cluster
- ...
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.
Filed #329
Guide uses reusable scripts from `bin/test-workflow`
8e79165
to
940b7bc
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.
Thanks for addressing those comments. LGTM, though I noticed there's no example output for some of the command but hardly a blocker.
@doodlesbykumbi I didn't include output for steps 6 and 7, they don't output any logs that succinctly indicate success. I'll file an issue to make them more descriptive in success. |
What does this PR do?
Includes
bin/test-workflow/QUICKSTART.md
Guide uses reusable scripts added in #307
The quickstart guide will only improve with updates
addressing #322 and #323 - guide will be able to divide
steps more clearly by persona responsability.
What ticket does this PR close?
Resolves #246
Checklists
Change log
Test coverage
Documentation
README
s) were updated in this PR, and/or there is a follow-on issue to update docs, orManual tests
If you are preparing for a release, have you run the following manual tests to verify existing functionality continues to function as expected?