Add event handler for custom notice; Add Juju actions #28

mvlassis · 2025-01-16T10:18:24Z

This PR adds the event handler for the PebbleCustomNotice event that syncs the Profiles based on the PMR found in the following path: <repository>/<pmr-yaml-path>.

Additionally, the 3 Juju actions sync-now, list-stale-profiles, and delete-stale-profiles have been implemented. They can be called with juju run github-profiles-automator/0 sync-now etc.

Tests

2 unit tests for _get_pmr_from_yaml() have been added. Note that we haven't added any integration tests, since the full functionality of create_or_update_profiles() of the profiles_management library has not been added.
5 integration tests have been added. The test the deployment of the charm, configuring it to change to active, and testing the 3 Juju actions sync-now, list-stale-profiles, and delete-stale-profiles.

kimwnasptd

Did a first quick pass, overall looks good, but also want to deploy it and test with the example PMR.

src/charm.py

mvlassis · 2025-01-21T14:21:51Z

Note that I am encountering the following issue as well: #38, which fortunately isn't a blockers as the integration tests still pass

kimwnasptd

Nice work @mvlassis!

One high level suggestion that I saw is that if we don't have trust in the charm, then it could error on config-changed and the status doesn't align. Let's look into if we could detect if the charm has trust via ops (although I doubt it).

If we can't, then maybe it'll make sense to have have an exception handling for ApiError with 403.

src/charm.py

tests/integration/test_charm.py

charmcraft.yaml

DnPlas

Thanks @mvlassis ! A couple comments.

src/charm.py

tests/integration/test_charm.py

tests/unit/test_charm.py

kimwnasptd

Took one more look, @mvlassis @DnPlas really good catch to also update profiles with update-status and config-changed!

Am OK from my side to not go with a component for now. Only a nit on whether we could avoid having multiple similar functions. But if you feel this doesn't add too much value feel free to resolve.

LGTM otherwise!

src/charm.py

DnPlas

Thanks @mvlassis, a second round of reviews! Left some comments.

src/charm.py

tests/integration/test_charm.py

tests/unit/test_charm.py

…yncing the profiles

Co-authored-by: Daniela Plascencia <[email protected]>

DnPlas

Thanks @mvlassis ! Just a thing about actions.

src/charm.py

DnPlas

Thanks @mvlassis !

mvlassis requested a review from a team as a code owner January 16, 2025 10:18

kimwnasptd mentioned this pull request Jan 16, 2025

Can't have Charm unit tests since the library initialises a client #29

Closed

mvlassis force-pushed the kf-6730-charm-implement-sync branch from f94689a to cc8c486 Compare January 16, 2025 12:16

kimwnasptd requested changes Jan 16, 2025

View reviewed changes

src/charm.py Show resolved Hide resolved

src/charm.py Outdated Show resolved Hide resolved

mvlassis force-pushed the kf-6730-charm-implement-sync branch 2 times, most recently from 27ebe9e to 19ff454 Compare January 21, 2025 11:54

kimwnasptd reviewed Jan 22, 2025

View reviewed changes

DnPlas reviewed Jan 22, 2025

View reviewed changes

charmcraft.yaml Outdated Show resolved Hide resolved

DnPlas reviewed Jan 22, 2025

View reviewed changes

mvlassis force-pushed the kf-6730-charm-implement-sync branch from e459734 to f41d16c Compare January 27, 2025 11:09

kimwnasptd reviewed Jan 27, 2025

View reviewed changes

src/charm.py Show resolved Hide resolved

src/charm.py Outdated Show resolved Hide resolved

kimwnasptd mentioned this pull request Jan 29, 2025

PMR: Create or Delete AuthorizationPolicies #39

Merged

DnPlas reviewed Jan 30, 2025

View reviewed changes

mvlassis force-pushed the kf-6730-charm-implement-sync branch from 172984d to 6fb3513 Compare January 30, 2025 11:02

mvlassis added 15 commits January 31, 2025 17:17

Add event handler for custom notice; Add Juju actions

18047e6

Fix linting

da674aa

Update unit test

325285b

Fix linting

3efcdf7

Update tests

1e762c7

Update tests

4d56229

Catch more error in _get_pmr_from_yaml()

e0fa4ea

Add more logs in Juju actions

6400644

Change lightkube client to property

ec2fd29

Update log comments

4e2411f

Add integration tests

e2fa627

Add integration tests

c369d04

Fix config changed event

c4abc16

Update status messages

072f604

Fix linting

db55eae

mvlassis and others added 19 commits January 31, 2025 17:17

Remove unecessary assertions for sync-now action

3dfcd74

Check the results of the list-stale-profile action in integration tests

6584799

Add mocked lightkube client to unit tests

240c19d

Fix linting

17232d2

Add type annotations for helper functions in charm integration tests

1aa3d49

Fix linting

a8b3191

Change test's name to be more fitting

b2430e9

Add check for whether we can connect to the Pebble container before s…

2dace33

…yncing the profiles

Load YAML files locally

67f7153

Update docstring

bb8e73a

Check if can connect to the container in _sync_profiles

c4ef81c

Update docstrings

d0c1309

Change sample profile in unit tests

6de1c29

First deploy all charms and then wait for them to become idle

34d9428

Remove unnecessary log messages

e14cd18

Remove unecessary wait_for_idle

4d9cfb0

Update tests/unit/test_charm.py

67b2c9c

Co-authored-by: Daniela Plascencia <[email protected]>

Update unit tests

a445d35

Turn integration tests to unit tests

c8798c6

mvlassis force-pushed the kf-6730-charm-implement-sync branch from 65f4bdb to c8798c6 Compare January 31, 2025 15:17

Add mocked lightkube client to action tests

a54be94

DnPlas reviewed Feb 3, 2025

View reviewed changes

src/charm.py Outdated Show resolved Hide resolved

mvlassis added 2 commits February 3, 2025 12:33

Handle ApiError in Juju actions

4a74911

Handle ErrorWithStatusError properly

4111963

DnPlas approved these changes Feb 3, 2025

View reviewed changes

kimwnasptd approved these changes Feb 3, 2025

View reviewed changes

kimwnasptd merged commit a9136ee into main Feb 3, 2025
9 checks passed

kimwnasptd deleted the kf-6730-charm-implement-sync branch February 3, 2025 12:08

This was referenced Feb 3, 2025

Implement updating the Profiles in the cluster from inside the charm code #24

Closed

Implement charm integration tests #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add event handler for custom notice; Add Juju actions #28

Add event handler for custom notice; Add Juju actions #28

mvlassis commented Jan 16, 2025 •

edited

Loading

kimwnasptd left a comment

mvlassis commented Jan 21, 2025

kimwnasptd left a comment

DnPlas left a comment

kimwnasptd left a comment

DnPlas left a comment

DnPlas left a comment

DnPlas left a comment

Add event handler for custom notice; Add Juju actions #28

Add event handler for custom notice; Add Juju actions #28

Conversation

mvlassis commented Jan 16, 2025 • edited Loading

Tests

kimwnasptd left a comment

Choose a reason for hiding this comment

mvlassis commented Jan 21, 2025

kimwnasptd left a comment

Choose a reason for hiding this comment

DnPlas left a comment

Choose a reason for hiding this comment

kimwnasptd left a comment

Choose a reason for hiding this comment

DnPlas left a comment

Choose a reason for hiding this comment

DnPlas left a comment

Choose a reason for hiding this comment

DnPlas left a comment

Choose a reason for hiding this comment

mvlassis commented Jan 16, 2025 •

edited

Loading