-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add UI, PostgreSQL, Minio and S3 relations #6
Conversation
Before I review the PR, I noticed something:
@kelkawi-a How does |
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 overall but not approving until the "issue" thread is resolved.
Minio on k8s stores on persistent storage. |
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.
left some minor comments and questions
it's nice to see this taking shape
great job!
try: | ||
charm = self.charm | ||
# Hack: get_interfaces checks for peer relation which does not exist under requires/provides list in charmcraft.yaml | ||
del charm.meta.relations["peer"] |
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.
so if you don't delete the peer
from meta, then what.. get_interfaces fails?
is there a way to get a specific interface... since you don't care about the others?
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.
Yes, get_interfaces
iterates through all the relations and searches for them under the list of relations defined in requires
and provides
, but the peer
relation is a special case which does not exist under them, so the method fails. Anyway, we do not care for searching for the peer
relation, hence why it can be removed.
@@ -0,0 +1,141 @@ | |||
# Copyright 2024 Canonical Ltd. |
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.
are these 2 files s3 and s3_helpers coming from somewhere? another charm?
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.
These are defined for this charm, but there are occurences of the helpers in other charms that relate to the s3 integrator as well (e.g. the create_bucket_if_not_exists
method)
This PR does the following:
Note: I've tested the functionality of adding source and destination connectors, and running a sync job. The only shortcoming thus far is that I am not able to get the Airbyte worker to store the logs in object storage (the bucket creation at the charm level works, Airbyte is just not communicating with the object storage properly to upload the logs). I am currently investigating this issue on different Airbyte slack channels and threads [1,2,3]. I have also tested the S3 integration with RadosGW, but Airbyte seems to have hardcoded this to AWS S3 storage, not just any S3-compatible storage. Regardless, this PR can be considered ready for review, with minor fixes to come potentially on some environment variables later.