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

fix: Add Persistence Agent ClusterRole and Binding #324

Merged
merged 6 commits into from
Sep 21, 2023

Conversation

phoevos
Copy link
Contributor

@phoevos phoevos commented Sep 12, 2023

Apply auth manifests (KF 1.8) for the KFP Persistence Agent, including the ClusterRole, ClusterRoleBinding, and ServiceAccount that allow the workload to get, list, and watch workflows/scheduledworkflows, as well as get namespaces.

Apply auth manifests for the KFP Persistence Agent, including the
ClusterRole, ClusterRoleBinding, and ServiceAccount that allow the
workload to get, list, and watch workflows/scheduledworkflows, as well
as get namespaces.

Signed-off-by: Phoevos Kalemkeris <phoevos.kalemkeris@canonical.com>
Signed-off-by: Phoevos Kalemkeris <phoevos.kalemkeris@canonical.com>
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @phoevos, this change makes sense. Is there a way we can expand the repository level integration tests to be able to catch this type of errors?

Co-authored-by: Daniela Plascencia <daniela.plascencia@canonical.com>
@phoevos
Copy link
Contributor Author

phoevos commented Sep 14, 2023

@DnPlas, I'm not really sure how we could catch this in our CI, given that this won't cause an issue right now (since we're deploying the charm with trust, the workload will by extension get all required permissions and then some). We could add a test to all of our sidecar charms to ensure that they also apply the Roles provided by upstream. But I think that the effort of doing that is exactly the same as just adding the required roles. Let alone the fact that if I remember to add the test, I'll realise that I need to apply some roles and I'll just do that.

Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

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

Thanks @phoevos LGTM!

@phoevos phoevos merged commit 4cef98a into main Sep 21, 2023
@phoevos phoevos deleted the kf-4351-add-persistence-roles branch September 21, 2023 10:15
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