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

feat: create k8s_service_info library #3

Merged
merged 12 commits into from
Mar 4, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Feb 7, 2024

This library provides functionality for sharing Kubernetes Service information using the k8s-service relation interface. Please refer to #2 for details.

NOTE: please merge after #4

Fixes #2

This library provides functionality for sharing Kubernetes Service information
using the k8s-service relation interface.

This commit adds the library and unit tests.

Fixes #2
@DnPlas DnPlas force-pushed the KF-5304-create-k8s-service-info-lib branch from d1074e0 to f8c17c5 Compare February 7, 2024 12:04
@DnPlas DnPlas marked this pull request as ready for review February 7, 2024 12:05
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
Copy link

@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.

I did a quick pass too and left some minor comments.

lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
@DnPlas DnPlas requested a review from a team as a code owner February 14, 2024 13:43
@orfeas-k
Copy link

Should we add some basic repo settings to this regarding merging and protection? Since this is something our charms will use. I see

# PR page
Require approval from specific reviewers before merging
# main repo page
Your main branch isn't protected
Protect this branch from force pushing or deletion, or require status checks before merging. [View documentation.](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets)

Copy link

@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.

added some comments but generally looks promising!

@DnPlas
Copy link
Contributor Author

DnPlas commented Feb 21, 2024

Should we add some basic repo settings to this regarding merging and protection? Since this is something our charms will use. I see

# PR page
Require approval from specific reviewers before merging
# main repo page
Your main branch isn't protected
Protect this branch from force pushing or deletion, or require status checks before merging. [View documentation.](https://docs.github.com/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets)

Good catch, I have added a branch protection rule for main.

Copy link

@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.

added some more review comments

tests/unit/test_k8s_service_info.py Outdated Show resolved Hide resolved
lib/charms/mlops_libs/v0/k8s_service_info.py Outdated Show resolved Hide resolved
tests/unit/test_k8s_service_info.py Show resolved Hide resolved
tests/unit/test_k8s_service_info.py Outdated Show resolved Hide resolved
tests/unit/test_k8s_service_info.py Show resolved Hide resolved
Copy link

@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.

Added a few things to look into as comments

Copy link

@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.

Small thing in the tests, otherwise lgtm

tests/unit/test_k8s_service_info.py Outdated Show resolved Hide resolved
Copy link

@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.

lgtm! ty @DnPlas!

@DnPlas DnPlas merged commit 985228f into main Mar 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create k8s_service_info library that replaces the SDI k8s-service interface
4 participants