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

Align FAR CRDs to NHC #16

Merged
merged 5 commits into from
Feb 23, 2023
Merged

Conversation

razo7
Copy link
Member

@razo7 razo7 commented Feb 21, 2023

FAR's CRDs didn't follow the motivation/desgin of the other remediators (e.g. MDRTemplate Spec) - remediator should have template field under Spec. This PR should align FAR CRDs to NHC/MHC expectations

  • Move the Spec fields from FARTemplate to FAR and let the FARTemplate to only have a template field under spec (of FenceAgentsRemediationSpec type).
  • Add Cluster Role for Role aggregation. Since that will give NHC/MHC access to the template and remediation CRs with a special label.
  • Update Reconcile according to this change.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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

@razo7 razo7 force-pushed the align-far-to-nhc branch 2 times, most recently from bde73aa to dabc652 Compare February 21, 2023 14:11
@razo7
Copy link
Member Author

razo7 commented Feb 21, 2023

Github CI has failed and #17 should resolve it.

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.

lgtm in general, two nits inside, will let Michael do a review as well

api/v1alpha1/fenceagentsremediation_types.go Show resolved Hide resolved
api/v1alpha1/fenceagentsremediationtemplate_types.go Outdated Show resolved Hide resolved
}

func (r *FenceAgentsRemediationReconciler) getFAPod(namespace string) (*corev1.Pod, error) {
// getFenceAgentsPod fetches the FAR pod based on FAR's label and namespace
func (r *FenceAgentsRemediationReconciler) getFenceAgentsPod(namespace string) (*corev1.Pod, error) {
Copy link
Member

Choose a reason for hiding this comment

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

something for a follow up, not this PR: you don't need to search the pod, you can know its namespace and name, see here for how to get the namespace, same should work for the name: https://github.com/medik8s/node-healthcheck-operator/blob/main/config/manager/manager.yaml#L53

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, and it sounds like a better coding

Align FARTemplete Spec with expected remediator spec for NHC
We need Role aggregation that will give NHC/MHC access to the template and remediation CRs with a special label
Due to CRDs changes the reconcile must be updated
Due to CRDs changes the CRD examples should be updated
Include CRD's resources in CSV
@mshitrit
Copy link
Member

I would have felt a lot better if we had some unit tests to verify this kind of a change, but I guess in this case it's an egg and chicken scenario 🙂
/lgtm

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.

4 participants