-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Destination Snowflake: set jdbc application env variable depends on env - airbyte_oss or airbyte_cloud #19302
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
Changes from 6 commits
eff8b5d
4a7cda8
729ba2a
ea58ba3
826e048
59c0b01
5a4c88e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/* | ||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||
*/ | ||
|
||
package io.airbyte.integrations.destination.snowflake; | ||
|
||
public class OssCloudEnvVarConsts { | ||
|
||
public static final String AIRBYTE_OSS = "airbyte_oss"; | ||
public static final String AIRBYTE_CLOUD = "airbyte_cloud"; | ||
|
||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,21 @@ | ||||||
/* | ||||||
* Copyright (c) 2022 Airbyte, Inc., all rights reserved. | ||||||
*/ | ||||||
|
||||||
package io.airbyte.integrations.destination.snowflake; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing /*
* Copyright (c) 2022 Airbyte, Inc., all rights reserved.
*/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added, thanks |
||||||
|
||||||
import static io.airbyte.integrations.destination.snowflake.SnowflakeDestination.SCHEDULED_EXECUTOR_SERVICE; | ||||||
|
||||||
import io.airbyte.integrations.base.adaptive.AdaptiveDestinationRunner; | ||||||
|
||||||
public class SnowflakeDestinationRunner { | ||||||
|
||||||
public static void main(final String[] args) throws Exception { | ||||||
AdaptiveDestinationRunner.baseOnEnv() | ||||||
.withOssDestination(() -> new SnowflakeDestination(OssCloudEnvVarConsts.AIRBYTE_OSS)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not blocking, maybe could have these variables imported at the top to keep in sync with Snowflake Source Lines 7 to 8 in 75d1353
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In one of the comments above, I was asked to downgrade it from Base level to destination one :) I already published the connector but will have a look at my next PR. At the moment when I started working on this PR we didn't have these vars. Thanks |
||||||
.withCloudDestination(() -> new SnowflakeDestination(OssCloudEnvVarConsts.AIRBYTE_CLOUD)) | ||||||
.run(args); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this probably also needs to do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated, thanks! |
||||||
SCHEDULED_EXECUTOR_SERVICE.shutdownNow(); | ||||||
} | ||||||
|
||||||
} |
This file was deleted.
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 be on the same line as above, right?
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.
For some reason, it started looking line that after "gradlew format" command (this command is also run in CI). I tried to make it a single line manually twice, but the automatic format sets it as you see now. Have no idea why
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.
In that case, we can keep this change and it appears you're already in the publication process plus it's not a functional requirement. Thanks for the context though