-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
airbyte-ci: tweaks for cross-repo compatibility #41541
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
421cb9d
to
f4d562a
Compare
f4d562a
to
9e92d81
Compare
9e92d81
to
5a0ec81
Compare
@@ -60,9 +60,6 @@ tasks.named('clean').configure { | |||
} | |||
|
|||
allprojects { | |||
// Evaluate CDK project before evaluating the connector. | |||
evaluationDependsOn(':airbyte-cdk:java:airbyte-cdk') | |||
|
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 isn't actually required at all in any case
found += project_dependencies | ||
if with_test_dependencies: | ||
found += test_dependencies | ||
return list(set(found)) |
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 function has been broken for a while
# Since first party project folders are transitive (compileOnly) in the | ||
# CDK, we always need to add them as the project dependencies. | ||
project_dependencies += get_local_cdk_gradle_dependencies(False) | ||
test_dependencies += get_local_cdk_gradle_dependencies(with_test_dependencies=True) |
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.
CDK projects haven't been compileOnly for a while
@@ -648,6 +622,7 @@ def __repr__(self) -> str: | |||
def get_local_dependency_paths(self, with_test_dependencies: bool = True) -> Set[Path]: | |||
dependencies_paths = [] | |||
if self.language == ConnectorLanguage.JAVA: | |||
dependencies_paths += [Path("./airbyte-cdk/java/airbyte-cdk")] |
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 imports all of the CDK source, which isn't that big in the first place, so this should be OK. This is required when connectors are built with useLocalCdk.= true
.
Let's run a test workflow on a Java an python connectors to make sure it does not break anything: |
Good call. I forgot to mention that I tested this on my machine. Testing on a github runner is better. Thanks for taking the initiative! |
0f41196
to
fad12cb
Compare
Co-authored-by: alafanechere <augustin.lafanechere@gmail.com>
What
This PR is a series of harmless changes which make it possible to re-use most of the tooling in this git repository in another git repository.
Review guide
Go through the PR one commit at a time.
User Impact
None whatsoever.
Can this PR be safely reverted and rolled back?