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

Remove temporary job to delete old monitoring stack, and fix kubebuilder RBAC #158

Merged

Conversation

israel-hdez
Copy link
Contributor

Description

This does two things:

  • Reverts add temp Job to delete old monitoring stack #146, since we no longer need the temporary job in main branch.
  • Fixes broken make manifests.
    • The make manifests is broken, because some privileges were manually added to config/rbac/role.yaml which should be an auto-generated file.
    • To fix, this PR adds the missing kubebuilder:rbac comments in main.go. Then make manifests is ran to properly re-generate the role.yaml file.
    • The version of controller-gen was bumped, to be able to include a header file making clear that the role.yaml file should not be changed manually.

How Has This Been Tested?

The revert of #146 removed a Job (manifest change), which can be seen clearly in the PR. It also changed the role.yaml file (another manifest change). Since the revert just involves changes to manifests, only build correctness was verified by running: ´kustomize build config/base/´ and verifying no errors.

The fix for make manifests involved regenerating the role.yaml file. To verify that privileges of the role remain the same:

  • Apply the role.yaml from main branch: oc apply -f https://raw.githubusercontent.com/opendatahub-io/odh-model-controller/main/config/rbac/role.yaml
  • Get role description: oc describe clusterrole odh-model-controller-role > role-before.txt
  • Apply the role.yaml from this PR: oc apply -f config/rbac/role.yaml. If should apply correctly, which would indicate a valid YAML.
  • Get new role description: oc describe clusterrole odh-model-controller-role > role-after.txt
  • Compare the two descriptions to check that they are equivalent: gvimdiff role-before role-after.txt. You will see something like this (before at left, after at right):

image

You will notice that 3 lines at the bottom are being removed from the role. That's OK because those privileges were needed only by the temporary Job that does cleanup of the old monitoring stack. Since the Job is being removed (the revert) those privileges are no longer needed.

You will also notice that the privileges for the ServiceAccounts is simply relocated with the same re-ordered privileges.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
    • NOTE: It is not squashed: 1 commit for the revert, and 1 to fix make manifests.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

…)"

This reverts commit 50c596a.

Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>
Some privileges in the controller role were added manually.

This adds the needed kubebuilder comments to have these roles properly included when running `make manifests`.

Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>
Copy link
Contributor

@VedantMahabaleshwarkar VedantMahabaleshwarkar left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Jan 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: israel-hdez, VedantMahabaleshwarkar

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 [VedantMahabaleshwarkar,israel-hdez]

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

@openshift-merge-bot openshift-merge-bot bot merged commit 062439d into opendatahub-io:main Jan 26, 2024
3 of 5 checks passed
@israel-hdez israel-hdez deleted the fix-rbac-and-revert branch January 26, 2024 16:36
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.

2 participants