Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: test scripts and readme doc about uninstallation #330

Merged
merged 1 commit into from
Aug 21, 2023
Merged

fix: test scripts and readme doc about uninstallation #330

merged 1 commit into from
Aug 21, 2023

Conversation

lihongyan1
Copy link
Contributor

fix #315

@lihongyan1 lihongyan1 requested a review from a team as a code owner August 9, 2023 07:28
@lihongyan1 lihongyan1 changed the title fix:test scripts and readme doc about uninstallation fix: test scripts and readme doc about uninstallation Aug 9, 2023
simonpasquier
simonpasquier reviewed Aug 9, 2023
View reviewed changes
README.md Outdated
Comment on lines 48 to 49
for crd in $(oc get crd|grep 'monitoring\.rhobs'|awk '{print $1}'); do \
oc delete --ignore-not-found customresourcedefinitions $crd; done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(suggestion) a bit shorter

Suggested change
for crd in $(oc get crd|grep 'monitoring\.rhobs'|awk '{print $1}'); do \
oc delete --ignore-not-found customresourcedefinitions $crd; done
kubectl delete crds $(kubectl api-resources --api-group=monitoring.rhobs -o name)

README.md Outdated
@@ -76,6 +80,9 @@ kubectl delete -n operators \
-l operators.coreos.com/observability-operator.operators=

kubectl delete -f hack/olm/k8s

for crd in $(kubectl get crd|grep 'monitoring\.rhobs'|awk '{print $1}'); do \
kubectl delete --ignore-not-found customresourcedefinitions $crd; done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

README.md
@@ -36,14 +36,18 @@ It is easier to use the web console to remove the installed operator.
Instructions below removes all traces of what was setup in the previous step
including removing the catalog.
```
kubectl delete -n operators csv \
oc delete -n operators csv \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up we could replace kubectl by oc across the whole OpenShift section.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally +1, it'll help being specific to openshift, although kubectl would work as well :D

test/run-e2e-ocp.sh Outdated
@@ -64,6 +64,8 @@ delete_obo() {

oc delete -f hack/olm/subscription.yaml || true
oc delete -f hack/olm/catalog-src.yaml || true
for crd in $(oc get crd|grep 'monitoring\.rhobs'|awk '{print $1}'); do \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

simonpasquier
simonpasquier previously approved these changes Aug 10, 2023
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@simonpasquier
Copy link
Contributor

Just one small fix to make the lint check green ;)

 In ./test/run-e2e-ocp.sh line 67:
make: *** [Makefile:38: lint-shell] Error 123
	oc delete crds $(oc api-resources --api-group=monitoring.rhobs -o name)
                       ^-- SC2046 (warning): Quote this to prevent word splitting.

danielmellado
danielmellado reviewed Aug 10, 2023
View reviewed changes
test/run-e2e-ocp.sh Outdated
@@ -64,6 +64,7 @@ delete_obo() {

oc delete -f hack/olm/subscription.yaml || true
oc delete -f hack/olm/catalog-src.yaml || true
oc delete crds $(oc api-resources --api-group=monitoring.rhobs -o name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Simon comments, please change this to let the linter go green, thanks!

Suggested change
oc delete crds $(oc api-resources --api-group=monitoring.rhobs -o name)
oc delete crds "$(oc api-resources --api-group=monitoring.rhobs -o name)"

@lihongyan1 lihongyan1 force-pushed the fix-unstall-doc branch 2 times, most recently from 37ee1e7 to 2c04529 Compare August 14, 2023 03:44
@lihongyan1
Copy link
Contributor Author

@simonpasquier @danielmellado Please help review and approve

@jan--f jan--f merged commit fca1667 into rhobs:main Aug 21, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmellado danielmellado danielmellado left review comments

@jan--f jan--f jan--f approved these changes

@simonpasquier simonpasquier simonpasquier left review comments

@slashpai slashpai Awaiting requested review from slashpai slashpai is a code owner automatically assigned from rhobs/observability-operator-maintainers

@JoaoBraveCoding JoaoBraveCoding Awaiting requested review from JoaoBraveCoding JoaoBraveCoding is a code owner automatically assigned from rhobs/observability-operator-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

All the crd should be deleted when uninstall ObO
4 participants
@lihongyan1 @simonpasquier @jan--f @danielmellado