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

refactor kfp-api's on_remove to use KRH built-in delete #385

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

ca-scribner
Copy link
Contributor

Refactors kfp-api charm's on_remove() event handler to use KubernetesResourceHandler.delete() rather than its own removal procedure. This was done to:

  • standardise on a removal procedure with other charms
  • remove the need for the on_remove() handler to have a KRH object with full context. In future we will add another Kubernetes object here that requires relation data to render, and this relation data may not be available during the removal event

@misohu
Copy link
Member

misohu commented Nov 20, 2023

LGTM I see kfp-api integration failing because of mysql error fixed with this #386 just the bundle test is failing on test_create_and_monitor_recurring_run. Maybe we can try to rerun. If it occurs again lets open issue and we can merge this change.

@ca-scribner
Copy link
Contributor Author

We will rebase and retarget this on main after #362 is merged

Refactors kfp-api charm's `on_remove()` event handler to use KubernetesResourceHandler.delete() rather than its own removal procedure.  This was done to:
* use the same removal procedure as other charms
* remove the need for the `on_remove()` handler to have a KRH object with full context (in future we will add another Kubernetes object here that requires relation data to render, but the relation data is not guaranteed to be available during remove)
@ca-scribner ca-scribner force-pushed the KF-4984-use-krh-delete-on-remove branch from f6a6db8 to 2abf848 Compare November 20, 2023 18:55
@ca-scribner ca-scribner changed the base branch from 1.8-updates-dev-branch to main November 20, 2023 18:55
@ca-scribner
Copy link
Contributor Author

PR has been rebased on main and is ready for re-review.

Copy link
Contributor

@NohaIhab NohaIhab left a comment

Choose a reason for hiding this comment

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

LGTM

@ca-scribner ca-scribner merged commit 1926e0b into main Nov 22, 2023
46 checks passed
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