-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: remove connector secrets hack for --is-local #33972
Changes from 13 commits
dd156f9
16c3f11
89effae
6b9e572
b5f7056
270fc49
826eabc
729a0d1
6820f5c
7c2c0d7
0d07d1d
b67dc28
d07d294
a1c6d4b
57868eb
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 |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
import anyio | ||
import asyncclick as click | ||
import asyncer | ||
from dagger import Client, Config, Container, ExecError, File, ImageLayerCompression, Platform, QueryError, Secret | ||
from dagger import Client, Config, Container, ExecError, File, ImageLayerCompression, Platform, Secret | ||
from more_itertools import chunked | ||
|
||
if TYPE_CHECKING: | ||
|
@@ -101,13 +101,10 @@ async def get_file_contents(container: Container, path: str) -> Optional[str]: | |
Returns: | ||
Optional[str]: The file content if the file exists in the container, None otherwise. | ||
""" | ||
try: | ||
return await container.file(path).contents() | ||
except QueryError as e: | ||
if "no such file or directory" not in str(e): | ||
# this error could come from a network issue | ||
raise | ||
return None | ||
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() | ||
Comment on lines
+104
to
+107
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. The precondition here is that 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. I think that raising an exception is fine in that case, |
||
|
||
|
||
@contextlib.contextmanager | ||
|
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a clue why the import is so slow?
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.
It's not this import in particular, it's all of the transitive imports in
pipeline.py
and beyond.