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: use k8s_service_info lib instead of SDI #87

Merged
merged 8 commits into from
Apr 22, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Apr 17, 2024

Use the k8s_service_info for receiving the MLMD GRPC Service info instead of using the SDI, as it will stop being supported soon.

Fixes #76

NOTE for reviewers:

  • I have added a component for wrapping the k8s service info requirer because we need to:
  1. Have a way to return a status based on the relation conditions
  2. Because there is no other way to do logic that prevents errors in the init method of the charm. For instance, it would be weird to have a try/catch in the init method for ensuring the required relation is established.
  • Also note that this component could be placed in chisme, BUT that requires us to put charm libraries in the package, which is under discussion by the team.

Testing instructions

  1. Deploy the charm in this branch
  2. Deploy mlmd from channel latest/edge with trust
  3. Relate both charms
  4. Verify both charms go to active and idle
Test upgrade path

Because this charm was recently refactored from podspec to sidecar pattern, there will no be a migration from its previous stable version.

@DnPlas DnPlas requested a review from a team as a code owner April 17, 2024 11:08
@DnPlas DnPlas marked this pull request as draft April 17, 2024 11:26
@DnPlas DnPlas force-pushed the KF-5472-use-k8s-service-info-lib branch 2 times, most recently from ea87584 to e425c8d Compare April 17, 2024 11:46
Use the k8s_service_info for receiving the MLMD GRPC Service info instead of using
the SDI, as it will stop being supported soon.

Fixes #76
@DnPlas DnPlas force-pushed the KF-5472-use-k8s-service-info-lib branch from e425c8d to a2fb053 Compare April 17, 2024 12:57
@DnPlas DnPlas marked this pull request as ready for review April 17, 2024 13:06
requirements.in Outdated Show resolved Hide resolved
@ca-scribner
Copy link
Contributor

Created canonical/charmed-kubeflow-chisme#94 to capture the discussion about how we could reuse code like the Component here that needs a charm library

src/charm.py Outdated Show resolved Hide resolved
requirements.in Outdated Show resolved Hide resolved
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

tiny issue otherwise lgtm

Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Approving since the last comment @ca-scribner had is resolved now.

@DnPlas DnPlas requested a review from ca-scribner April 22, 2024 08:36
@DnPlas
Copy link
Contributor Author

DnPlas commented Apr 22, 2024

Approving since the last comment @ca-scribner had is resolved now.

Thanks @orfeas-k ! It looks like @ca-scribner has to approve it as he requested changes ;(

@DnPlas
Copy link
Contributor Author

DnPlas commented Apr 22, 2024

Approving since the last comment @ca-scribner had is resolved now.

Thanks @orfeas-k ! It looks like @ca-scribner has to approve it as he requested changes ;(

Dismissing @ca-scribner as the change he requested was very small and I did the change. Thanks both for the reviews!

@DnPlas DnPlas dismissed ca-scribner’s stale review April 22, 2024 11:53

The request was to change a duplicated line, which I did. This is a small enough change to not require another review.

@DnPlas DnPlas merged commit 155b30a into main Apr 22, 2024
7 checks passed
@DnPlas DnPlas deleted the KF-5472-use-k8s-service-info-lib branch April 22, 2024 11:53
@ca-scribner
Copy link
Contributor

👍 ty!

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.

update the grpc relation to use the mlops-libs k8s_service_info library
3 participants