-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: ClickPipelineContext handles dagger logging #33419
airbyte-ci: ClickPipelineContext handles dagger logging #33419
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
6ddfdf5
to
68e0adc
Compare
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.
Good change! Just have a question on where this should live.
if self.params.get("show_dagger_logs", False): | ||
log_output = sys.stdout | ||
else: | ||
log_output, self._click_context().obj["dagger_logs_path"] = self._create_dagger_client_log_file() |
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.
Nice change!
One house keeping thing
1. Move this logic to its own method
def get_log_output(self):
if self.params.get("show_dagger_logs", False):
log_output = sys.stdout
else:
log_output, self._click_context().obj["dagger_logs_path"] = self._create_dagger_client_log_file()
return log_output
@@ -97,6 +103,14 @@ async def get_dagger_client(self, pipeline_name: Optional[str] = None, log_outpu | |||
assert self._dagger_client, "Error initializing Dagger client" | |||
return self._dagger_client.pipeline(pipeline_name) if pipeline_name else self._dagger_client | |||
|
|||
def _create_dagger_client_log_file(self) -> Tuple[io.FileIO, str]: |
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.
❓ Instead of adding this logic here, could we not hoist it into airbyte_ci.py
or dagger_run.py
and make it shared between all pipelines?
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 something I considered but as the log file is tightly coupled to the client I prefer to keep it where the client is instantiated.
I'm also conscious that we currently have inconsistencies in the way we handle dagger client logging in our commands.
I created this issue to consolidate the approach.
TLDR; all our commands should get their dagger client from ClickPipelineContext
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.
That sounds fine then!
At that point in time we should be in a good place to introduce the GlobalSettings pattern from aircmd,
34beb12
to
3e1ad9e
Compare
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.
lgtm!
@@ -97,6 +103,14 @@ async def get_dagger_client(self, pipeline_name: Optional[str] = None, log_outpu | |||
assert self._dagger_client, "Error initializing Dagger client" | |||
return self._dagger_client.pipeline(pipeline_name) if pipeline_name else self._dagger_client | |||
|
|||
def _create_dagger_client_log_file(self) -> Tuple[io.FileIO, str]: |
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.
That sounds fine then!
At that point in time we should be in a good place to introduce the GlobalSettings pattern from aircmd,
3e1ad9e
to
fe819b4
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
What
ClickPipelineContext
users rely on theget_dagger_client
method to get the dagger client as a singleton.We originally exposed a
log_output
parameter to this method, but as it returns a singleton dagger client, the passedlog_output
is not taken into account if the singleton already exists.How
The
get_dagger_client
defines the log output path according to theshow_dagger_logs
params.If
show_dagger_logs
is true: show the logs in the stdout, otherwise log to a file.Next:
show_dagger_logs
param/options and always log to a filedagger run
wrapped commandClickPipelineContext
to have a consistent logging management implementation