-
Notifications
You must be signed in to change notification settings - Fork 162
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 a more comprehensive example #158
Conversation
464a1a8
to
e9e3303
Compare
/assign @barney-s I only added a bash script for deployment, but not for test, because the e2e test will be almost the same as wordpress. |
docs/examples/test-app/test_crd.yaml
Outdated
acceptedNames: | ||
kind: "" | ||
plural: "" | ||
conditions: [] |
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.
Could we add a condition struct array here ?
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 test includes CRD only without the controller, so the conditions wouldn't be updated and it makes no difference if we have a condition struct array or not. For simplicity, I would keep it empty.
docs/examples/test-app/deploy.sh
Outdated
make deploy | ||
|
||
# Install the test CRD to the cluster | ||
kubectl apply -f test_crd.yaml |
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.
We may have to wait for the crd to be registered.
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 will add a readln to confirm the CRD is registered.
docs/examples/test-app/deploy.sh
Outdated
kubectl apply -f application.yaml | ||
|
||
# Verify application's status/components field shows all components | ||
# Verify ownerReferences is added to all components' spec |
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.
Does the script wait by itself ?
Do we need a readln OR a sleep here ?
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.
We validate the components' specs and application's status manually, but I can add a readln in the script just in case we may want to automate the script later.
please take a look at #159 |
awesome! Do we want to add a new e2e test for this comprehensive case? Or do we keep the bash script for now? @barney-s |
name: "test-application-01" | ||
labels: | ||
app.kubernetes.io/name: "test-01" | ||
spec: |
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.
Let's encourage to use descriptor, especially once it's integrated with kubectl/kubernetes-dashboard that would be the first experience user get and missing fields wouldn't look good on those tools.
The new example includes apps.Deployment, batch.Job, Service, ConfigMap and a custom CRD.
65cd9c3
to
a44fb66
Compare
@barney-s ready to be re-reviewed and merged |
Thanks for updating. If you want to do a follow up PR also it ok. |
/lgtm we can reorg the files in a follow up PR |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: barney-s, nan-yu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The new example includes apps.Deployment, batch.Job, Service, ConfigMap and a custom CRD.