-
Notifications
You must be signed in to change notification settings - Fork 7
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
CSS-4803 - Vault relation fixes and extra logging #1002
Conversation
- Also added readme file with manual steps for deploying JIMM for future reference.
- Vault access address now comes from config as it would be difficult to determine programatically - Added test to ensure JIMM enters an error state
aa68871
to
743af33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM bar some questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.. but i think the charm should go into blocked state if the vault-access-address is not specified.. what do you think?
@@ -0,0 +1,48 @@ | |||
# Description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should really live in the jaas-documentation repo..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm good point. I think we can leave it here for now and I'll move it in a follow-up
@@ -499,9 +507,13 @@ def _get_network_address(self, event): | |||
return str(self.model.get_binding(event.relation).network.egress_subnets[0].network_address) | |||
|
|||
def _on_vault_relation_joined(self, event): | |||
if self.config.get("vault-access-address") is None: | |||
logger.error("Missing config vault-access-address for vault relation") | |||
raise ValueError("Missing config vault-access-address for vault relation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just enter into blocked status if this is the case? and defer the event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I've set it to go into an error
state instead is so that it doesn't keep re-retrying endlessly and instead you can set the missing config and then run juju resolve <charm>
which will rerun the hook. Maybe that's not the right way to handle it?
Description
Engineering checklist
Check only items that apply