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

Emit events during remediation process #96

Merged
merged 7 commits into from
Jan 9, 2024
Merged

Emit events during remediation process #96

merged 7 commits into from
Jan 9, 2024

Conversation

clobrano
Copy link
Contributor

@clobrano clobrano commented Oct 2, 2023

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 2, 2023

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 Oct 2, 2023
@clobrano
Copy link
Contributor Author

clobrano commented Oct 2, 2023

/test ?

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 2, 2023

@clobrano: The following commands are available to trigger required jobs:

  • /test 4.12-ci-bundle-machine-deletion-remediation-bundle
  • /test 4.12-images
  • /test 4.12-openshift-e2e
  • /test 4.12-unit
  • /test 4.13-ci-bundle-machine-deletion-remediation-bundle
  • /test 4.13-images
  • /test 4.13-openshift-e2e
  • /test 4.13-unit
  • /test 4.14-ci-bundle-machine-deletion-remediation-bundle
  • /test 4.14-images
  • /test 4.14-openshift-e2e
  • /test 4.14-unit

Use /test all to run all jobs.

In response to this:

/test ?

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.

@clobrano
Copy link
Contributor Author

clobrano commented Oct 2, 2023

/test 4.14-openshift-e2e

@clobrano clobrano force-pushed the add_events/0 branch 5 times, most recently from e27982f to d41b31b Compare October 5, 2023 08:41
@clobrano
Copy link
Contributor Author

clobrano commented Oct 6, 2023

/test 4.14-openshift-e2e

@clobrano clobrano marked this pull request as ready for review October 6, 2023 08:50
@openshift-ci openshift-ci bot requested review from beekhof and razo7 October 6, 2023 08:51
slintes
slintes requested changes Oct 9, 2023
View reviewed changes
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

I think we are missing events in the if machine == nil {} block, otherwise 👍🏼

@clobrano clobrano marked this pull request as draft December 20, 2023 10:07
@clobrano clobrano changed the title Emit events during remediation process [WIP] Emit events during remediation process Dec 20, 2023
@clobrano
Copy link
Contributor Author

Converting the PR to WIP to prevent tests execution

@clobrano clobrano marked this pull request as ready for review January 2, 2024 11:43
@razo7 razo7 marked this pull request as ready for review January 4, 2024 15:27
@clobrano clobrano changed the title Emit events during remediation process [WIP] Emit events during remediation process Jan 4, 2024
@openshift-ci openshift-ci bot requested a review from mshitrit January 4, 2024 15:27
@clobrano clobrano marked this pull request as draft January 4, 2024 15:28
@clobrano
Copy link
Contributor Author

clobrano commented Jan 4, 2024

moved to draft to avoid wasting tests

@openshift-ci openshift-ci bot removed the lgtm label Jan 4, 2024
@clobrano
Copy link
Contributor Author

clobrano commented Jan 4, 2024

/test 4.14-openshift-e2e

@clobrano clobrano changed the title [WIP] Emit events during remediation process Emit events during remediation process Jan 4, 2024
@razo7
Copy link
Member

razo7 commented Jan 8, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 8, 2024
@razo7 razo7 marked this pull request as ready for review January 8, 2024 15:34
@openshift-ci openshift-ci bot requested a review from razo7 January 8, 2024 15:34
slintes
slintes requested changes Jan 8, 2024
View reviewed changes
Copy link
Member

@slintes slintes left a comment

Choose a reason for hiding this comment

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

one comment inside, otherwise lgtm

controllers/machinedeletionremediation_controller.go Outdated
@@ -225,7 +245,8 @@ func (r *MachineDeletionRemediationReconciler) Reconcile(ctx context.Context, re
return ctrl.Result{}, errors.Wrapf(err, "failed to save Machine's name and namespace")
}

log.Info("request machine deletion", "machine", machine.GetName())
commonevents.NormalEvent(r.Recorder, mdr, machineDeletionRequestedEventReason, machineDeletionRequestedEventMessage)
Copy link
Member

@slintes slintes Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this this event says "requested" (past tense), so it should emitted after successful machine deletion

@openshift-ci openshift-ci bot removed the lgtm label Jan 8, 2024
@openshift-ci openshift-ci bot added the lgtm label Jan 9, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano, 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:

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

@slintes
Copy link
Member

slintes commented Jan 9, 2024

/hold cancel

@openshift-merge-bot openshift-merge-bot bot merged commit 0ba5428 into medik8s:main Jan 9, 2024
19 checks passed
@clobrano clobrano deleted the add_events/0 branch January 9, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slintes slintes slintes approved these changes

@beekhof beekhof Awaiting requested review from beekhof

@mshitrit mshitrit Awaiting requested review from mshitrit

@razo7 razo7 Awaiting requested review from razo7

Assignees

@slintes slintes

@razo7 razo7

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@clobrano @razo7 @slintes @openshift-merge-robot