-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Logging to other Cloud Storages. #4501
Conversation
Nice. Skimmed this. I like the direction. |
@jrhizor besides the test, this is almost there. want to give it a look over? |
*/ | ||
boolean hasEmptyConfigs(Configs configs); | ||
static boolean hasEmptyConfigs(Configs configs) { |
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 is not a great interface to pass Configs
. You're passing too much data and with a lot that has nothing to do with the logging feature
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.
A class that groups the s3 fields or gcp fields into individual objects would help achieve this.
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.
Good point. I did think about this but didn't think it was too confusing at the time.
I created a logging configuration interface that has a subset of the logging related config methods. The implementation to the logging configuration interface delgates to the configs interface so force a dependency to avoid multiple ways of configuration the same thing.
airbyte-config/models/src/main/java/io/airbyte/config/helpers/CloudLogs.java
Outdated
Show resolved
Hide resolved
airbyte-config/models/src/main/java/io/airbyte/config/helpers/GcsLogs.java
Outdated
Show resolved
Hide resolved
airbyte-config/models/src/main/java/io/airbyte/config/helpers/S3Logs.java
Outdated
Show resolved
Hide resolved
*/ | ||
boolean hasEmptyConfigs(Configs configs); | ||
static boolean hasEmptyConfigs(Configs configs) { |
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.
A class that groups the s3 fields or gcp fields into individual objects would help achieve this.
What
#4200
How
Describe the solution
Recommended reading order
x.java
y.python
Pre-merge Checklist
Expand the checklist which is relevant for this PR.
Connector checklist
airbyte_secret
in the connector's spec./gradlew :airbyte-integrations:connectors:<name>:integrationTest
./test connector=connectors/<name>
command as documented here is passing.README.md
docs/SUMMARY.md
if it's a new connectordocs/integrations/<source or destination>/<name>
.docs/integrations/...
. See changelog exampledocs/integrations/README.md
contains a reference to the new connector/publish
command described hereConnector Generator checklist
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes