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

Set the status on different exceptions that are raised #45

Merged
merged 6 commits into from
Feb 12, 2025

Conversation

kimwnasptd
Copy link
Contributor

Closes #25

Right now the Charm will go in a blocked status, but a lot of times it won't have a helpful error message. This PR improves the error handling to also set a status, but not when an action is triggered.

Changes

  1. Update the non-action handlers that call _sync_profiles() to set the status of the charm
  2. Update _sync_profiles() to raise ErrorWithStatus on API errors

Review Notes

Here's a following sequence of commands I tried and the corresponding charm statuses. In all cases shown the charm was in Blocked state.

juju deploy \
	./github-profiles-automator_amd64.charm \
	--resource \
	git-sync-image=registry.k8s.io/git-sync/git-sync:v4.4.0
# Config `repository` cannot be empty.

juju config \
	github-profiles-automator \
	repository=https://github.com/canonical/github-profiles-automator.git
#  Could not load YAML file at path pmr.yaml. You may need to configure `pmr-yaml-path`. Check the logs for more informa...

juju config \
	github-profiles-automator \
	pmr-yaml-path=tests/samples/pmr-sample-full.yaml
# Charm is missing required permissions. Make sure it has --trust.

juju config \
	github-profiles-automator \
	git-revision=incorrect-branch
# [git-sync-pebble-service] git-sync could not connect to the repository. You may need to configure the charm or add an...

juju config \
	github-profiles-automator \
	git-revision=main
# Charm is missing required permissions. Make sure it has --trust.

juju trust --scope=cluster github-profiles-automator
# Becase active

@kimwnasptd kimwnasptd requested a review from a team as a code owner February 12, 2025 10:26
@kimwnasptd kimwnasptd force-pushed the kf-6731-exceptions-handling branch from d2ca4a1 to b465b91 Compare February 12, 2025 10:50
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
kimwnasptd and others added 2 commits February 12, 2025 14:08
Co-authored-by: Manos Vlassis <[email protected]>
Co-authored-by: Manos Vlassis <[email protected]>
src/charm.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mvlassis mvlassis left a comment

Choose a reason for hiding this comment

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

Minor comments, nice work!

Copy link
Collaborator

@mvlassis mvlassis left a comment

Choose a reason for hiding this comment

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

Nice stuff :)

@kimwnasptd kimwnasptd merged commit 383b083 into main Feb 12, 2025
9 checks passed
@kimwnasptd kimwnasptd deleted the kf-6731-exceptions-handling branch February 12, 2025 13:11
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.

PMR: Handle exceptions that might be raised and return proper charm status
2 participants