You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "utkarsharma2 (via GitHub)" <gi...@apache.org> on 2023/10/05 13:14:39 UTC
[PR] Fix autodetect_docker_context for list of dict case [airflow]
utkarsharma2 opened a new pull request, #34779:
URL: https://github.com/apache/airflow/pull/34779
In breeze's autodetect_docker_context() there is one case that missed, when the output of `docker context ls --format=json"` is `[{"Name": "desktop-linux", "DockerEndpoint": "unix://desktop-linux"}]`. This PR aims to handle this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #34779:
URL: https://github.com/apache/airflow/pull/34779#issuecomment-1749180184
As follow up I guess we could try to run this step by use [python-on-whales](https://github.com/gabrieldemarmiesse/python-on-whales) so many problems happen last time with this part and `--format=json"`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #34779:
URL: https://github.com/apache/airflow/pull/34779
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on PR #34779:
URL: https://github.com/apache/airflow/pull/34779#issuecomment-1749579118
@Taragolis I can give it a shot. :)
Just a confirmation do we update all the similar instances with equvilant in [python-on-whales](https://github.com/gabrieldemarmiesse/python-on-whales) or just this instance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #34779:
URL: https://github.com/apache/airflow/pull/34779#discussion_r1347477172
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -828,7 +828,12 @@ def autodetect_docker_context():
if result.returncode != 0:
get_console().print("[warning]Could not detect docker builder. Using default.[/]")
return "default"
- context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
+ try:
+ context_dicts = json.loads(result.stdout)
+ if isinstance(context_dicts, dict):
+ context_dicts = [context_dicts]
+ except json.decoder.JSONDecodeError:
+ context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
Review Comment:
Thanks @ephraimbuddy Much elegant solution. :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #34779:
URL: https://github.com/apache/airflow/pull/34779#discussion_r1347443286
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -828,7 +828,12 @@ def autodetect_docker_context():
if result.returncode != 0:
get_console().print("[warning]Could not detect docker builder. Using default.[/]")
return "default"
- context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
+ try:
+ context_dicts = json.loads(result.stdout)
+ if isinstance(context_dicts, dict):
+ context_dicts = [context_dicts]
+ except json.decoder.JSONDecodeError:
+ context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
Review Comment:
I don't think this is the issue. The issue is at the known_context. It should be:
```
known_contexts = {item["Name"]: item for info in context_dicts for item in info}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix autodetect_docker_context for list of dict case [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #34779:
URL: https://github.com/apache/airflow/pull/34779#discussion_r1347603278
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -828,7 +828,12 @@ def autodetect_docker_context():
if result.returncode != 0:
get_console().print("[warning]Could not detect docker builder. Using default.[/]")
return "default"
- context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
+ try:
+ context_dicts = json.loads(result.stdout)
+ if isinstance(context_dicts, dict):
+ context_dicts = [context_dicts]
+ except json.decoder.JSONDecodeError:
+ context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
Review Comment:
Upon closer look I found:
Below code generates list of dicts for `'{"Name": "default", "DockerEndpoint": "unix://default"}\n{"Name": "a", "DockerEndpoint": "unix://a"}'`
```
context_dicts = (json.loads(line) for line in result.stdout.splitlines() if line.strip())
Sample code:
result = _fake_ctx_output("default", 'a')
context_dicts = (json.loads(line) for line in result.splitlines() if line.strip())
for test in context_dicts:
print(test)
```
Output:
`{'Name': 'default', 'DockerEndpoint': 'unix://default'}
{'Name': 'a', 'DockerEndpoint': 'unix://a'}`
But for the new input the data in never converted to list of dicts. Remains the list
```
result = '[{"Name": "desktop-linux", "DockerEndpoint": "unix://desktop-linux"}]'
context_dicts = (json.loads(line) for line in result.splitlines() if line.strip())
for test in context_dicts:
print(test)
```
Output:
`[{'Name': 'desktop-linux', 'DockerEndpoint': 'unix://desktop-linux'}]`
And fails in the next steps which assumes the data to be list of dicts.
earlier solution earlier converts both input to list of dicts:
```
def test_inputs(result):
try:
context_dicts = json.loads(result)
if isinstance(context_dicts, dict):
context_dicts = [context_dicts]
except json.decoder.JSONDecodeError:
context_dicts = (json.loads(line) for line in result.splitlines() if line.strip())
for item in context_dicts:
print(item)
```
Output:
```
test_inputs('{"Name": "default", "DockerEndpoint": "unix://default"}\n{"Name": "a", "DockerEndpoint": "unix://a"}')
{'Name': 'default', 'DockerEndpoint': 'unix://default'}
{'Name': 'a', 'DockerEndpoint': 'unix://a'}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org