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.

Sign up for GitHub

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

Jump to bottom

feat: resource dispatcher charm library #42

Merged
merged 19 commits into from
Jan 22, 2024
Merged

feat: resource dispatcher charm library #42

merged 19 commits into from
Jan 22, 2024

Conversation

NohaIhab
Copy link
Contributor

@NohaIhab NohaIhab commented Jan 15, 2024 •
edited
Loading

closes #41

  • Implements a charm library for resource dispatcher to handle the relations: pod-defaults, secrets, service-accounts
  • uses the provider side of the library in the resource dispatcher charm code
  • re-writes the unit tests to match the new implementation
  • re-writes the manifests-tester charm to use the requirer side of the new library

@NohaIhab NohaIhab requested a review from a team as a code owner January 15, 2024 14:09
ca-scribner
ca-scribner suggested changes Jan 16, 2024
View reviewed changes
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.

Had some comments, but overall lgtm! Only main concern is the comment in the tester charm where I think we will have a problem using this when the kubernetes resources are not statically defined

lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Outdated Show resolved Hide resolved
tests/integration/manifests-tester/src/charm.py Outdated Show resolved Hide resolved
tests/integration/manifests-tester/lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
metadata.yaml Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
DnPlas
DnPlas reviewed Jan 17, 2024
View reviewed changes
Copy link
Collaborator

@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 @NohaIhab ! This is looking great already, I have left some comments.

lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
lib/charms/resource_dispatcher/v0/resource_dispatcher.py Outdated Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
tests/unit/test_resource_dispatcher_lib.py Show resolved Hide resolved
ca-scribner
ca-scribner previously approved these changes Jan 19, 2024
View reviewed changes
@ca-scribner
Copy link

Should also get a review from @DnPlas but lgtm

DnPlas
DnPlas reviewed Jan 19, 2024
View reviewed changes
Copy link
Collaborator

@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 @NohaIhab, I did another pass. It's looking good, minor nits here and there.

lib/charms/kubernetes_manifests/v0/kubernetes_manifests.py Show resolved Hide resolved
lib/charms/kubernetes_manifests/v0/kubernetes_manifests.py Outdated Show resolved Hide resolved
lib/charms/kubernetes_manifests/v0/kubernetes_manifests.py Show resolved Hide resolved
lib/charms/kubernetes_manifests/v0/kubernetes_manifests.py Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
tests/integration/test_charm.py Outdated Show resolved Hide resolved
tests/unit/test_resource_dispatcher_lib.py Show resolved Hide resolved
DnPlas
DnPlas approved these changes Jan 22, 2024
View reviewed changes
Copy link
Collaborator

@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 @NohaIhab, LGTM!

@NohaIhab NohaIhab merged commit fa9c190 into main Jan 22, 2024
6 checks passed
@NohaIhab NohaIhab deleted the kf-5184-feat-resource-dispatcher-lib branch January 22, 2024 16:44
@NohaIhab NohaIhab mentioned this pull request Jan 30, 2024
Merged
@NohaIhab NohaIhab mentioned this pull request Mar 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DnPlas DnPlas DnPlas approved these changes

@ca-scribner ca-scribner ca-scribner left review comments

Assignees
No one assigned
Labels
Libraries: Out of sync
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement a charm lib for pod-defaults relation
3 participants
@NohaIhab @ca-scribner @DnPlas