airbyte-ci: remove connector secrets hack for --is-local #33972
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -24,6 +23,8 @@ def deploy(ctx: click.Context) -> None: | |||
| @deploy.command(cls=DaggerPipelineCommand, name="orchestrator", help="Deploy the metadata service orchestrator to production") | |||
| @click.pass_context | |||
| async def deploy_orchestrator(ctx: click.Context) -> None: | |||
| # Import locally to speed up CLI. | |||
| from pipelines.airbyte_ci.metadata.pipeline import run_metadata_orchestrator_deploy_pipeline | |||
There was a problem hiding this comment.
This shaves 4 seconds off of the execution time for airbyte-ci --help, bringing it down to under a second.
There was a problem hiding this comment.
Do you have a clue why the import is so slow?
There was a problem hiding this comment.
It's not this import in particular, it's all of the transitive imports in pipeline.py and beyond.
| @@ -24,6 +23,8 @@ def deploy(ctx: click.Context) -> None: | |||
| @deploy.command(cls=DaggerPipelineCommand, name="orchestrator", help="Deploy the metadata service orchestrator to production") | |||
| @click.pass_context | |||
| async def deploy_orchestrator(ctx: click.Context) -> None: | |||
| # Import locally to speed up CLI. | |||
| from pipelines.airbyte_ci.metadata.pipeline import run_metadata_orchestrator_deploy_pipeline | |||
There was a problem hiding this comment.
Do you have a clue why the import is so slow?
| # this error could come from a network issue | ||
| raise | ||
| return None | ||
| nonexistence = await container.with_exec(["sh", "-c", f"[ -f '{path}' ] || echo '{path} does not exist'"]).stdout() |
There was a problem hiding this comment.
This consider that:
shis available in the container- the container entrypoint is not overidden (you could set
skip_entrypoint=Trueon thewith_exec)
I'd find it more robust to only rely on Dagger's API to check file existence.
Maybe something like:
- Inferring the directory from the path
- Call
await container.directory(directory_path).entries()and check if the file is in the returned list.
There was a problem hiding this comment.
Yeah, I didn't think this one through. Good catch!
|
Thanks for the review, this is ready for another look. |
| dir_name, file_name = os.path.split(path) | ||
| if file_name not in set(await container.directory(dir_name).entries()): | ||
| return None | ||
| return await container.file(path).contents() |
There was a problem hiding this comment.
The precondition here is that path is a file path and not a dir path, I'm not sure its enforced but I'm fine with it, Dagger will raise an exception if the path leads to a directory.
There was a problem hiding this comment.
I think that raising an exception is fine in that case, get_file_contents really makes no sense if applied to a directory.
|
Thanks for the review! I resolved the conflicts, waiting for ✅ |
|
Bypassing branch protections owing to the failing formatting job, which is failing because it can't provision the right kind of runner or something. |
postamar
deleted the
postamar/airbyte-ci-remove-local-secrets-special-casing
branch
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
See #33582 (comment) for context. I tested that these changes worked by running airbyte-ci locally.