Skip to content

Conversation

@vedantlodha
Copy link
Contributor


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 for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

Copy link
Member

@pankajastro pankajastro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if it makes sense to use AirflowConsole, it would allow print in multiple formats.

@vedantlodha
Copy link
Contributor Author

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.

@vedantlodha vedantlodha requested a review from pankajastro June 12, 2023 11:51
@pankajastro
Copy link
Member

pankajastro commented Jun 12, 2023

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 formats for connection export like connection export - --file-format yaml but not for variables export command. so maybe it ok for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use more meaningful variable names instead of qry, var, val?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. I just refactored the helper function into this. Will update the patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
except Exception:
except json.decoder.JSONDecodeError:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always a good idea to handle specific exceptions as close to the source as possible.

Copy link
Contributor

@utkarsharma2 utkarsharma2 Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this was just to maintain some consistency with connection_command.py's export. Will update the patch

@vedantlodha
Copy link
Contributor Author

Thanks for the reviews @utkarsharma2. Updated the patch. Please take a look when you have some time.

@vedantlodha vedantlodha requested a review from utkarsharma2 June 13, 2023 06:31
Comment on lines 82 to 90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
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?

Copy link
Member

@uranusjr uranusjr Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop the all() as well since it’s only iterated once. Also why is the decoder called the data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay, had been unwell lately.

Thanks for the inputs. Ive updated the patch.

 Similar to connections, this change allows the cli to export the variables to the stdout using 'airflow variables export -'

 Fixes apache#31851
- handle JSONDecodeError instead of Exception
var_dict[row.key] = value

with args.file as f:
f.write(json.dumps(var_dict, sort_keys=True, indent=4))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f.write(json.dumps(var_dict, sort_keys=True, indent=4))
json.dump(f, var_dict, sort_keys=True, indent=4)

@github-actions
Copy link

github-actions bot commented Aug 4, 2023

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.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Aug 4, 2023
@github-actions github-actions bot closed this Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow variables to be printed to STDOUT

4 participants