You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vedantlodha (via GitHub)" <gi...@apache.org> on 2023/06/12 09:48:44 UTC
[GitHub] [airflow] vedantlodha opened a new pull request, #31854: Allow variables to be printed to STDOUT with airflow variables export.
vedantlodha opened a new pull request, #31854:
URL: https://github.com/apache/airflow/pull/31854
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #31851
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
This change allows airflow cli to export variables to stdout similar to connections using `airflow variables export -`.
Fixes #31851
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
[GitHub] [airflow] github-actions[bot] commented on pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31854:
URL: https://github.com/apache/airflow/pull/31854#issuecomment-1666044001
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227444689
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
Review Comment:
Would be better if we treat the absence of value in `args.file` as an indication to use stdout instead of equating the name with `args.file.name == "<stdout>"`. WDYT?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227438927
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
+ val = var.val
+ var_dict[var.key] = val
+ file_is_stdout = _is_stdout(args.file)
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if file_is_stdout:
+ print("\nVariables successfully exported.", file=sys.stderr)
+ else:
+ print(f"Variables successfully exported to {args.file.name}.")
+
+
+def _is_stdout(fileio: io.TextIOWrapper) -> bool:
Review Comment:
This method doesn't really add much value, I would directly use `fileio.name == "<stdout>"` in line 96. Saves one lookup to a different method. :)
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227437103
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
Review Comment:
```suggestion
except json.decoder.JSONDecodeError:
```
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228846964
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
Review Comment:
combined it with previous comments
--
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
[GitHub] [airflow] vedantlodha commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "vedantlodha (via GitHub)" <gi...@apache.org>.
vedantlodha commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227593155
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
+ val = var.val
+ var_dict[var.key] = val
+ file_is_stdout = _is_stdout(args.file)
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if file_is_stdout:
+ print("\nVariables successfully exported.", file=sys.stderr)
+ else:
+ print(f"Variables successfully exported to {args.file.name}.")
+
+
+def _is_stdout(fileio: io.TextIOWrapper) -> bool:
Review Comment:
Sure, this was just to maintain some consistency with connection_command.py's export. Will update the patch
--
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
[GitHub] [airflow] vedantlodha commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "vedantlodha (via GitHub)" <gi...@apache.org>.
vedantlodha commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227592403
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
Review Comment:
Fair point. I just refactored the helper function into this. Will update the patch.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227438927
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
+ val = var.val
+ var_dict[var.key] = val
+ file_is_stdout = _is_stdout(args.file)
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if file_is_stdout:
+ print("\nVariables successfully exported.", file=sys.stderr)
+ else:
+ print(f"Variables successfully exported to {args.file.name}.")
+
+
+def _is_stdout(fileio: io.TextIOWrapper) -> bool:
Review Comment:
This method doesn't really add much value, I would directly use `fileio.name == "<stdout>"` in line 96. Saves one lookup.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1229100836
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
+ except json.decoder.JSONDecodeError:
+ value = variable.val
+ var_dict[variable.key] = value
Review Comment:
I think we can drop the `all()` as well since it’s only iterated once.
--
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
[GitHub] [airflow] vedantlodha commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "vedantlodha (via GitHub)" <gi...@apache.org>.
vedantlodha commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1234995735
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
+ except json.decoder.JSONDecodeError:
+ value = variable.val
+ var_dict[variable.key] = value
Review Comment:
Apologies for the delay, had been unwell lately.
Thanks for the inputs. Ive updated the patch.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228848575
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
+ except json.decoder.JSONDecodeError:
+ value = variable.val
+ var_dict[variable.key] = value
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if args.file.name == "<stdout>":
Review Comment:
Also, would it make more sense to use the absence of `args.file` to indicate that we can print data to stdout?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228847858
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
+ except json.decoder.JSONDecodeError:
+ value = variable.val
+ var_dict[variable.key] = value
Review Comment:
```suggestion
rows = session.query(Variable).all()
data = json.JSONDecoder()
for row in rows:
try:
value = data.decode(row.val)
except json.decoder.JSONDecodeError:
value = row.val
var_dict[row.key] = value
```
Maybe something like this?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227445978
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
+ val = var.val
+ var_dict[var.key] = val
+ file_is_stdout = _is_stdout(args.file)
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if file_is_stdout:
+ print("\nVariables successfully exported.", file=sys.stderr)
+ else:
+ print(f"Variables successfully exported to {args.file.name}.")
+
+
+def _is_stdout(fileio: io.TextIOWrapper) -> bool:
+ return fileio.name == "<stdout>"
Review Comment:
Would it be better if we treat the absence of argument `file` as an indication to use the print output to `stdout`. WDYT?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228845111
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
Review Comment:
```suggestion
rows = session.query(Variable).all()
```
Since session.query(Variable).all() returns rows of data. :)
--
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
[GitHub] [airflow] vedantlodha commented on pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "vedantlodha (via GitHub)" <gi...@apache.org>.
vedantlodha commented on PR #31854:
URL: https://github.com/apache/airflow/pull/31854#issuecomment-1588629407
Thanks for the reviews @utkarsharma2. Updated the patch. Please take a look when you have some time.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228845111
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
Review Comment:
```suggestion
rows = session.query(Variable).all()
```
Since this returns rows of data. :)
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228845111
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
Review Comment:
```suggestion
rows = session.query(Variable).all()
```
Since this returns rows of data. :)
--
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
[GitHub] [airflow] pankajastro commented on pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on PR #31854:
URL: https://github.com/apache/airflow/pull/31854#issuecomment-1587314843
> If by console, you mean the UI, that makes sense, however this allows the cli to do so as well, similar to connections.
>
> In the past i have had to switch between the cli and the UI for connections and variables.
In stdout only. I was thinking if we could print in different formats like json/yaml etc
But I see we do print in different for connection export like `connection export - --file-format yaml` but not for variables export command. so maybe it ok for now.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1235019108
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ rows = session.query(Variable)
+
+ decoder = json.JSONDecoder()
+ for row in rows:
+ try:
+ value = decoder.decode(row.val)
+ except json.decoder.JSONDecodeError:
Review Comment:
```suggestion
except JSONDecodeError:
```
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227435295
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
Review Comment:
Can we please use more meaningful variable names instead of qry, var, val?
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1229100836
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
+ except json.decoder.JSONDecodeError:
+ value = variable.val
+ var_dict[variable.key] = value
Review Comment:
I think we can drop the `all()` as well since it’s only iterated once. Also why is the decoder called the data?
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228846788
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
+ try:
+ value = data.decode(variable.val)
Review Comment:
```suggestion
value = data.decode(row.val)
```
--
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
[GitHub] [airflow] github-actions[bot] closed pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
URL: https://github.com/apache/airflow/pull/31854
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227437492
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
Review Comment:
It's always a good idea to handle specific exceptions as close to the source as possible.
--
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
[GitHub] [airflow] vedantlodha commented on pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "vedantlodha (via GitHub)" <gi...@apache.org>.
vedantlodha commented on PR #31854:
URL: https://github.com/apache/airflow/pull/31854#issuecomment-1587100569
If by console, you mean the UI, that makes sense, however this allows the cli to do so as well, similar to connections.
In the past i have had to switch between the cli and the UI for connections and variables.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1227438927
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +78,29 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ qry = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for var in qry:
+ try:
+ val = data.decode(var.val)
+ except Exception:
+ val = var.val
+ var_dict[var.key] = val
+ file_is_stdout = _is_stdout(args.file)
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
+ if file_is_stdout:
+ print("\nVariables successfully exported.", file=sys.stderr)
+ else:
+ print(f"Variables successfully exported to {args.file.name}.")
+
+
+def _is_stdout(fileio: io.TextIOWrapper) -> bool:
Review Comment:
This method doesn't really add much value, I would directly use `fileio.name == "<stdout>"` in line 96.
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228845890
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
+
+ data = json.JSONDecoder()
+ for variable in query:
Review Comment:
```suggestion
for row in rows:
```
and this combined with comment - https://github.com/apache/airflow/pull/31854/files#r1228845111
--
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
[GitHub] [airflow] utkarsharma2 commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1228845111
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ query = session.query(Variable).all()
Review Comment:
```suggestion
rows = session.query(Variable).all()
```
Since this returns rows of data. :)
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #31854: Allow variables to be printed to STDOUT with airflow variables export.
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #31854:
URL: https://github.com/apache/airflow/pull/31854#discussion_r1235019980
##########
airflow/cli/commands/variable_command.py:
##########
@@ -76,7 +77,24 @@ def variables_import(args):
def variables_export(args):
"""Exports all the variables to the file."""
- _variable_export_helper(args.file)
+ var_dict = {}
+ with create_session() as session:
+ rows = session.query(Variable)
+
+ decoder = json.JSONDecoder()
+ for row in rows:
+ try:
+ value = decoder.decode(row.val)
+ except JSONDecodeError:
+ value = row.val
+ var_dict[row.key] = value
+
+ with args.file as f:
+ f.write(json.dumps(var_dict, sort_keys=True, indent=4))
Review Comment:
```suggestion
json.dump(f, var_dict, sort_keys=True, indent=4)
```
--
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