Skip to content

Adding Uninstall script for issue 168 #179

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
Closed
wants to merge 10 commits into from
Closed

Adding Uninstall script for issue 168 #179

wants to merge 10 commits into from

Conversation

tosin2013
Copy link
Contributor

Adding clean up script for issue 168
#168

@openshift-ci openshift-ci bot requested review from copejon and fzdarsky July 23, 2021 12:34
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 23, 2021

Hi @tosin2013. Thanks for your PR.

I'm waiting for a redhat-et member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2021
@rootfs
Copy link
Member

rootfs commented Jul 23, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rootfs

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2021
@rootfs
Copy link
Member

rootfs commented Jul 23, 2021

/assign @oglok

guymguym
guymguym reviewed Jul 27, 2021
View reviewed changes
Copy link

@guymguym guymguym left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@rootfs
Copy link
Member

rootfs commented Jul 27, 2021

thank you @guymguym
wdyt @oglok

@rootfs
Copy link
Member

rootfs commented Jul 27, 2021

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 27, 2021
oglok
oglok previously requested changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@oglok oglok left a comment

Choose a reason for hiding this comment

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

Thanks @tosin2013 for pushing this PR. We really appreciate your contributions.

copejon
copejon reviewed Aug 6, 2021
View reviewed changes
jbpratt
jbpratt reviewed Sep 19, 2021
View reviewed changes
Copy link
Contributor

@jbpratt jbpratt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Should the microshift binary also be removed from the system? Cleaning up firewall rules, selinux config, stopping crio and uninstalling packages (if the desire is to have this script clean up everything from the install script) may be needed as well.

@cooktheryan
Merge branch 'main' into main
6e610e5
@cooktheryan
Update cleanup.sh
6a5e0a4 
additional cleanup items based on feedback
@cooktheryan cooktheryan requested review from oglok and copejon October 15, 2021 15:25
@cooktheryan cooktheryan dismissed oglok’s stale review October 15, 2021 15:38

i believe these are solved with the latest push

@cooktheryan
Copy link
Contributor

/retest

@cooktheryan cooktheryan requested review from guymguym and removed request for guymguym, copejon and fzdarsky October 15, 2021 19:17
guymguym
guymguym reviewed Oct 19, 2021
View reviewed changes
Copy link

@guymguym guymguym left a comment

Choose a reason for hiding this comment

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

@cooktheryan @tosin2013 With @sallyom adding RPM building for microshift, would we use a hacky script like this? or is RPM uninstall going to call this one?

@cooktheryan
Copy link
Contributor

@guymguym I would say with RPM we would have a more formal removal process. This is in my opinion a nice supplement to help when running the install.sh or doing local development to set you back at a close to beginning state

guymguym
guymguym reviewed Oct 19, 2021
View reviewed changes
hack/cleanup.sh
Comment on lines +83 to +87
V ${SUDO} rm -rf /var/lib/rook
V ${SUDO} rm -rf /var/lib/etcd
V ${SUDO} rm -rf /var/lib/kubelet

V ${SUDO} mkdir -p /var/lib/kubelet
Copy link

Choose a reason for hiding this comment

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

@rootfs WDYT Does it make sense to delete all these dirs here? Can there be any harm to a user that does not know that we do this cleanup? I think these are all "data" dirs, and the reason you used those originally was to make sure we remember to reset the state of rook/etcd/kubelet between install test cycles...

@guymguym
Copy link

@guymguym I would say with RPM we would have a more formal removal process. This is in my opinion a nice supplement to help when running the install.sh or doing local development to set you back at a close to beginning state

👍 Sounds good @cooktheryan !

jbpratt
jbpratt reviewed Oct 26, 2021
View reviewed changes
@netlify
Copy link

netlify bot commented Oct 27, 2021 •
edited
Loading

❌ Deploy Preview for microshift failed.

🔨 Explore the source changes: e5b7fe7

🔍 Inspect the deploy log: https://app.netlify.com/sites/microshift/deploys/6179963af443970007acad41

@cooktheryan cooktheryan mentioned this pull request Oct 31, 2021
Closed
@cooktheryan
Copy link
Contributor

closed will merge items in at #405

@cooktheryan cooktheryan closed this Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@copejon copejon copejon left review comments

@oglok oglok Awaiting requested review from oglok

+2 more reviewers

@guymguym guymguym guymguym left review comments

@jbpratt jbpratt jbpratt left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@oglok oglok

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tosin2013 @rootfs @cooktheryan @guymguym @oglok @copejon @jbpratt