enforce order of installing python dependencies #1149
Conversation
This reverts commit 5a4eb3b.
|
Ran standard test for shopify (failed) and adwords (passed). I'm bumping adwords after this gets approved but I'll bump shopify as part of #1120 |
|
pushed adwords 0.1.4 (and re-running the previous action) |
This reverts commit caee9ee.
| @@ -1,7 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
why these getting deleted?
There was a problem hiding this comment.
These aren't present in source_definitions.yaml. I guess I should add them if they were just missing when that was set up?
| @@ -10,3 +10,5 @@ airbytePython { | |||
| dependencies { | |||
| implementation files(project(':airbyte-integrations:bases:airbyte-protocol').airbyteDocker.outputs) | |||
| } | |||
|
|
|||
| installReqs.dependsOn(":airbyte-integrations:bases:airbyte-protocol:installReqs") | |||
There was a problem hiding this comment.
how did you think about picking :installReqs versus just depending on the build of that module? i'm wondering if we are going to make a bad assumption in the future. i'm also fine with keeping this though if you think it's the right way.
There was a problem hiding this comment.
This should go away in the future, and it may go away in favor of the build step. However, the build step in the future might not match what we have now, so I think this is the lightest weight shim to tide us over.
| @@ -12,3 +12,10 @@ airbytePython { | |||
| dependencies { | |||
| implementation files(project(':airbyte-integrations:bases:base-singer').airbyteDocker.outputs) | |||
| } | |||
|
|
|||
| // used to allow local iteration to work | |||
There was a problem hiding this comment.
so should we be doing this in all singer sources or just this one if just this one can you explain why in the comment?
There was a problem hiding this comment.
if all of them do we need to document it? put it in the template, etc. okay if this happens in a separae commit if that's the case.
There was a problem hiding this comment.
Ideally we do all of them, but it's lower priority than getting something working. Since we only had two cases where it didn't align, and both are easily fixable with this, it seems easiest to do this here. If we split it for every singer source, we shouldn't do it here; it should be part of a singer integration plugin.
| @@ -48,23 +49,25 @@ class AirbytePythonPlugin implements Plugin<Project> { | |||
| command = "install -r requirements.txt" | |||
| inputs.file('requirements.txt') | |||
| outputs.file('build/installedlocalreqs.txt') | |||
| outputs.cacheIf { true } | |||
|
|
|||
| // HACK: makes all integrations depend on installing requirements for bases. long term we should resolve deps and install in order. | |||
There was a problem hiding this comment.
long term we should resolve deps and install in order.
what will that look like? should we just have an airbyte-integration.gradle that handles this stuff? again. doesn't have to happen now, just want to understand how you're thinking about it and if possible create an issue with those thoughts.
if you add issue also add its number to the comment.
There was a problem hiding this comment.
I'm still figuring out precisely how to do this. I'll post on here later with the improvement ticket, but this HACK should be gone by EOW.
fixes #1031
fixes #1110 (mostly due to #1111). The acceptance criteria are tangential to the main issue and need to be addressed separately, I'll make a separate ticket.
The main issue was that we weren't waiting for parent dependency pip installs (or protocol generation) before running child dependency installs. This is a bit hacky here, will be cleaned up in a later PR.
Also, to fix other build problems, this PR: