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.

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

Update docs with the new remediationStrategy spec #145

Merged
merged 7 commits into from
May 22, 2024

Conversation

clobrano
Copy link
Contributor

@clobrano clobrano commented May 9, 2024

Why we need this PR

FAR CR has a new spec field remediationStrategy (see #92)

Changes made

Add the documentation about the new FAR remediation's spec field remediationStrategy

Copy link
Contributor

openshift-ci bot commented May 9, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label May 9, 2024
@clobrano clobrano requested a review from razo7 May 9, 2024 15:23
Copy link
Member

@razo7 razo7 left a comment

Choose a reason for hiding this comment

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

Please link to the PR which introduced the new field and mention that it is not only modifying README, as it should also update the CR sample as in #122

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@clobrano clobrano changed the title Update README with the new remediationStrategy spec Update docs with the new remediationStrategy spec May 13, 2024
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@clobrano clobrano force-pushed the update-readme-out-of-service-taint-0 branch from 7c38251 to a2de242 Compare May 14, 2024 16:08
README.md Outdated Show resolved Hide resolved
@clobrano clobrano force-pushed the update-readme-out-of-service-taint-0 branch from a2de242 to 3ff441e Compare May 15, 2024 07:48
@razo7
Copy link
Member

razo7 commented May 20, 2024

/test 4.16-openshift-e2e

@clobrano
Copy link
Contributor Author

 level=fatal msg=error destroying bootstrap resources failed during the destroy bootstrap hook: failed to remove bootstrap SSH rule: bootstrap ssh rule was not removed within 5m0s: timed out waiting for the condition 

The error doesn't seem our fault. Retrying

/test 4.16-openshift-e2e

@razo7
Copy link
Member

razo7 commented May 20, 2024

/retest

1 similar comment
@clobrano
Copy link
Contributor Author

/retest

@clobrano clobrano marked this pull request as ready for review May 21, 2024 11:26
@openshift-ci openshift-ci bot requested review from razo7 and slintes May 21, 2024 11:26
@clobrano
Copy link
Contributor Author

/retest

@clobrano
Copy link
Contributor Author

/test 4.12-openshift-e2e

@clobrano
Copy link
Contributor Author

/test 4.14-openshift-e2e

README.md Outdated
@@ -192,6 +192,9 @@ The CR includes the following parameters:
* `retrycount` - number of times to retry the fence agent in case of failure. The default is 5.
* `retryinterval` - interval between retries in seconds. The default is "5s".
* `timeout` - timeout for the fence agent in seconds. The default is "60s".
* `remediationStrategy` - either `ResourceDeletion` or `OutOfServiceTaint`:
* `ResourceDeletion`: This remediation strategy removes the pods on the node.
Copy link
Member

@slintes slintes May 21, 2024

Choose a reason for hiding this comment

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

Maybe it's me only, but why don't we call things how they are called already? 🤔
Here: we do not remove anything, we delete them... that's the actual method name we use. That's the D in CRUD. 🤷🏼‍♂️ (Same below)

(and I don't block on this, just a general thought, which I had more often at various places)

Copy link
Member

Choose a reason for hiding this comment

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

/lgtm

in case you want to change...
/hold

Copy link
Contributor Author

@clobrano clobrano May 22, 2024

Choose a reason for hiding this comment

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

why don't we call things how they are called already

for me, I think it's just an automatic mechanism to avoid repetitions in written documents, but indeed when explaining something this habit can be limited :)

Use "delete" in place of "remove" as it is more appropriate, and put
Out-of-service taint first in the list.

Signed-off-by: Carlo Lobrano <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label May 22, 2024
@openshift-ci openshift-ci bot added the lgtm label May 22, 2024
@clobrano
Copy link
Contributor Author

/test 4.13-openshift-e2e

@clobrano
Copy link
Contributor Author

/test 4.16-openshift-e2e

@clobrano
Copy link
Contributor Author

/unhold

Copy link
Contributor

openshift-ci bot commented May 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, razo7, slintes

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:
  • OWNERS [clobrano,razo7,slintes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@clobrano clobrano merged commit fb0a3ed into medik8s:main May 22, 2024
22 checks passed
@clobrano clobrano deleted the update-readme-out-of-service-taint-0 branch May 22, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants