-
Notifications
You must be signed in to change notification settings - Fork 54
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
📖 Fix docs: "Getting Started" and "Tutorials" #1443
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1443 +/- ##
==========================================
- Coverage 74.91% 74.69% -0.22%
==========================================
Files 42 42
Lines 3241 3241
==========================================
- Hits 2428 2421 -7
- Misses 642 647 +5
- Partials 171 173 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
It is already quite a lot of changes. I think I'll do the rest of the docs in a separate PR. Marking as ready for review. |
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.
/hold
Let's create a PR for each fix. With so many changes, it is too hard to review them all.
Furthermore, it is doing many more changes than one doc.
I would suggest each PR has a goal (i.e Fix specific issue A in Y because Z) and not be something generic like "Fix many issues across the project"
@camilamacedo86 I appreciate that this is difficult to review and I'm usually advocating of small PRs. However on the authoring side it is a lot of tedious manual work here. I spent most of the day today going through docs and verifying the snippets and outputs. And there a lot more to do. Coordinating this work across multiple PRs will add serious amount of overhead. But I see that this PR is already large - that's why I'm stopping here and will do the rest of the docs in a separate PR. |
/unhold |
I left a few small comments, otherwise the changes look good to me |
@@ -28,42 +28,40 @@ kind: ClusterExtension | |||
metadata: | |||
name: argocd | |||
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.
If you open this doc
1 - It should be using kubectl apply -f - <<EOF
so that we can apply it
2 - The command should have the value instead of <extension_name>
( as you changed in the other places )
kubectl patch clusterextension <extension_name> --type='merge' -p '{"spec": {"source": {"catalog": {"version": "<target_version>"}}}}'
3 - The verification step should either have the name ( why are you changing one place and not the others? )
kubectl get clusterextension.olm.operatorframework.io/<extension_name> -o yaml
$ kubectl get clusterextension argocd -o yaml
apiVersion: olm.operatorframework.io/v1alpha1
kind: ClusterExtension
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"olm.operatorframework.io/v1alpha1","kind":"ClusterExtension","metadata":{"annotations":{},"name":"argocd"},"spec":{"namespace":"argocd","serviceAccount":{"name":"argocd-installer"},"source":{"catalog":{"packageName":"argocd-operator","version":"0.6.0"},"sourceType":"Catalog"}}}
creationTimestamp: "2024-11-11T16:50:53Z"
generation: 4
name: argocd
resourceVersion: "42713"
uid: 4bcaf118-a4cf-4013-842f-501a9da7f597
spec:
namespace: argocd
serviceAccount:
name: argocd-installer
source:
catalog:
packageName: argocd-operator
upgradeConstraintPolicy: CatalogProvided
version: 0.6.0
sourceType: Catalog
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.
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.
Updated. Note that I did not change number 1 to use kubectl apply -f - <<EOF
: the snippet you are referring to is a pre-requisite. An example of the original state before upgrade.
Resolving,
2fccea6
to
7fd5eac
Compare
There were several intentional breaking changes in the API which are now included in v0.18.0 release. This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project. Relevant PRs: * operator-framework#1439 * operator-framework#1434 Signed-off-by: Mikalai Radchuk <[email protected]>
Re-opening for review. Note: we will still need to iterate over "Getting Started" and "Tutorials" sections. See to-dos in the PR descriptions. |
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 going through and updating these @m1kola - the changes LGTM!
9c394f1
Description
There were several intentional breaking changes in the API which are now included in v0.18.0 release.
This commit mostly focuses on updating the documentation to reflect API changes. This includes making sure that snippets and example outputs match the current state of the project.
Relevant PRs:
.spec.install.namespace
and.spec.install.serviceAccount
to.spec.namespace
and.spec.serviceAccount
#1439Still need to address (in next parts):
AllNamespaces
install mode and do not use webhooks" query here and here. Update: see Fix "Return list of packages that supportAllNamespaces
install mode and do not use webhooks" catalog query in the docs #1460Reviewer Checklist