Update documentation with deploy script and other minor changes#256
Conversation
Reviewer's GuideUpdates the getting-started documentation to include explicit environment setup, cluster lifecycle steps, KubeVirt installation, and a new one-shot deployment script for quickly standing up a test environment. Flow diagram for one-shot deploy script in getting-started guideflowchart TD
A[Start one-shot deploy script] --> B[Set environment exports
CONTAINER_CLI, RUNTIME
AK_REGISTRATION_ADDR
TRUSTEE_ADDR
REGISTRY]
B --> C[make cluster-down]
C --> D[make cluster-up]
D --> E[make install-kubevirt]
E --> F[make push]
F --> G[make manifests]
G --> H[make install]
H --> I[kubectl -n trusted-execution-clusters get po,svc]
I --> J[sleep 10m]
J --> K[examples/create-ignition-secret.sh
examples/ignition-coreos.json
coreos-ignition-secret]
K --> L[kubectl apply -f examples/vm-coreos-ign.yaml]
L --> M[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The one-shot deploy script currently relies on a hardcoded
sleep 10m; consider replacing this with a readiness check (e.g.,kubectl waitor checking specific pods) so the script fails fast if the cluster or operator never becomes ready. - There are a few small typos in the new content (e.g.,
Kube-virtvsKubeVirt,oparator exports,deplyoed) that should be corrected for clarity and consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The one-shot deploy script currently relies on a hardcoded `sleep 10m`; consider replacing this with a readiness check (e.g., `kubectl wait` or checking specific pods) so the script fails fast if the cluster or operator never becomes ready.
- There are a few small typos in the new content (e.g., `Kube-virt` vs `KubeVirt`, `oparator exports`, `deplyoed`) that should be corrected for clarity and consistency.
## Individual Comments
### Comment 1
<location path="docs/usage/getting-started-guide.md" line_range="91" />
<code_context>
```
This example works with KubeVirt when the KBS is reachable using the pod networking.
+Make sure Kube-virt is also installed before trying to install the operator and testing functionality.
+```console
+make install-kubevirt
</code_context>
<issue_to_address>
**suggestion (typo):** Use the correct project name capitalization (KubeVirt).
Also remove the hyphen so it matches the upstream spelling.
```suggestion
Make sure KubeVirt is also installed before trying to install the operator and testing functionality.
```
</issue_to_address>
### Comment 2
<location path="docs/usage/getting-started-guide.md" line_range="115" />
<code_context>
+Refer to the one-shot deploy script at the end of this README for a quick install once you've understood the process.
+
Further customization of the project can be controlled with the following env variables:
+ NAMESPACE: sets the namespace where the operator will be deplyoed
+ PLATFORM: use during the installation to configure the platform where the operator will be deployed (`kind` or `openshift`)
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo in "deplyoed".
In the NAMESPACE description, update the word to "deployed".
```suggestion
+ NAMESPACE: sets the namespace where the operator will be deployed
```
</issue_to_address>
### Comment 3
<location path="docs/usage/getting-started-guide.md" line_range="176" />
<code_context>
+export CONTAINER_CLI=docker
+export RUNTIME=docker
+
+# oparator exports
+export AK_REGISTRATION_ADDR=attestation-key-register.trusted-execution-clusters.svc.cluster.local
+export TRUSTEE_ADDR=kbs-service.trusted-execution-clusters.svc.cluster.local
</code_context>
<issue_to_address>
**issue (typo):** Fix the misspelling of "operator" in this comment.
Currently spelled "oparator"; please correct it to "operator".
```suggestion
# operator exports
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@Jakob-Naucke For some reason I cannot add reviewers, unsure whether its a permission issue. Is /cc the correct way? |
|
/cc @Jakob-Naucke |
|
/cc @alicefr |
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
Thank you for the improvements! Some small comments.
|
|
||
| Also make sure the container CLI environment variable is configured | ||
| ```console | ||
| export CONTAINER_CLI=docker |
There was a problem hiding this comment.
Building should work fine on Podman -- did you find this to be necessary?
There was a problem hiding this comment.
There is a line in the guide saying:
Kind can be used with docker or podman. Although, we set podman as default, right now there are some networking issues, therefore, the suggested runtime to use is docker at the moment. The runtime can be configured with:
So I went ahead and set default of runtime and container_cli to docker.
If the aforementioned issue is resolved, I'll change it to podman/CRI agnostic.
There was a problem hiding this comment.
Ah yes. CONTAINER_CLI is really just used for building and pushing, but this deserves clarification so let's maybe remove the paragraph but put this in its place:
If you require a non-Podman engine for building and pushing images, you can override it with the `$CONTAINER_CLI` variable.There was a problem hiding this comment.
building and pushing can be either podman or docker, but for the runtime env variable? That has to be docker due to networking issues on podman?
There was a problem hiding this comment.
Exactly, that's what the paragraph just above is about
|
|
||
| Wait for cluster to be ready: | ||
| ```console | ||
| sleep 10m |
There was a problem hiding this comment.
do we really need to put a sleep in the doc?
There was a problem hiding this comment.
Something we could add is a Ready Condition in the TEC CR and wait for it. @Jakob-Naucke WDYT?
It exists, and it's on me for not thinking of it earlier…
| sleep 10m | |
| kubectl wait -n trusted-execution-clusters --for=condition=Installed TrustedExecutionCluster trusted-execution-cluster |
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
please also squash the removal and the sourcery suggestions commits
|
|
||
| Wait for cluster to be ready: | ||
| ```console | ||
| sleep 10m |
There was a problem hiding this comment.
Something we could add is a Ready Condition in the TEC CR and wait for it. @Jakob-Naucke WDYT?
It exists, and it's on me for not thinking of it earlier…
| sleep 10m | |
| kubectl wait -n trusted-execution-clusters --for=condition=Installed TrustedExecutionCluster trusted-execution-cluster |
SpaceFace02
force-pushed
the
docs/COCL-362
branch
from
fd2ad05 to
1d5a01d
Compare
SpaceFace02
force-pushed
the
docs/COCL-362
branch
from
1d5a01d to
33f7e9c
Compare
@Jakob-Naucke Just a side-note, Do we have the squash on merge enabled for this repo? |
It's enabled, but we've always created merge commits. So far, the practice has been to rewrite the history and force-push, so in the case of this PR, you'd squash the "Changes based on review comments" commit you've just added too (but we can also squash-and-merge just this one and you don't need to force-push if all's well). I prefer this approach because it makes reverts easier, even though it makes reviewing changes harder (but FWIW GitHub allows to view the changes by clicking the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jakob-Naucke, SpaceFace02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Jakob-Naucke
merged commit f7b5b4d
into
trusted-execution-clusters:main
3 participants
Added deploy script + minor changes
Summary by Sourcery
Update the getting started guide to document a full one-shot deployment flow and required environment/setup steps for creating and using a kind-based cluster with the operator.
Enhancements:
Documentation: