You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "ephraimbuddy (via GitHub)" <gi...@apache.org> on 2023/08/30 20:20:06 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request, #33932: Fix updating variables during variable imports
ephraimbuddy opened a new pull request, #33932:
URL: https://github.com/apache/airflow/pull/33932
We should only create new variables during variable imports and not update already existing 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] ephraimbuddy commented on a diff in pull request #33932: Fix updating variables during variable imports
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312796294
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,18 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
+ if action_on_existing == "fail":
+ existing_key = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
+ if existing_key:
+ raise SystemExit(f"Failed. These keys: {list(sorted(existing_key))} already exists.")
+
for k, v in var_json.items():
+ if action_on_existing == "skip":
+ if session.scalar(select(Variable).where(Variable.key == k)):
+ skipped.add(k)
+ continue
Review Comment:
This would mean that even if the user chose to overwrite, we will still run the existing key query because the query will no longer be under an if statement
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312788415
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,18 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
+ if action_on_existing == "fail":
+ existing_key = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
+ if existing_key:
+ raise SystemExit(f"Failed. These keys: {list(sorted(existing_key))} already exists.")
Review Comment:
```suggestion
raise SystemExit(f"Failed. These keys: {sorted(existing_key)} already exists.")
```
--
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] ephraimbuddy commented on a diff in pull request #33932: Fix updating variables during variable imports
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312799653
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,18 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
+ if action_on_existing == "fail":
+ existing_key = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
+ if existing_key:
+ raise SystemExit(f"Failed. These keys: {list(sorted(existing_key))} already exists.")
+
for k, v in var_json.items():
+ if action_on_existing == "skip":
+ if session.scalar(select(Variable).where(Variable.key == k)):
+ skipped.add(k)
+ continue
Review Comment:
Is that better?
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1311124218
##########
airflow/www/views.py:
##########
@@ -5158,6 +5164,12 @@ def varimport(self):
flash(f"{suc_count} variable(s) successfully updated.")
if fail_count:
flash(f"{fail_count} variable(s) failed to be updated.", "error")
+ if skipped:
+ flash(
+ f"The variables with these keys: {list(skipped)} were skipped "
Review Comment:
```suggestion
skipped_repr = ", ".join(repr(k) for k in sorted(skipped))
f"The variables with these keys: {skipped_repr} were skipped "
```
We should this in the web UI so some formatting would be a good idea
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312670104
##########
airflow/cli/commands/variable_command.py:
##########
@@ -97,6 +107,8 @@ def variables_import(args):
print(f"{suc_count} of {len(var_json)} variables successfully updated.")
if fail_count:
print(f"{fail_count} variable(s) failed to be updated.")
+ if skipped:
+ print(f"The variables with these keys: {list(skipped)} were skipped because they already exists")
Review Comment:
It would be a good idea to sort this for output stability. It would be easier to test as well.
--
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] ephraimbuddy merged pull request #33932: Fix updating variables during variable imports
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy merged PR #33932:
URL: https://github.com/apache/airflow/pull/33932
--
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] potiuk commented on pull request #33932: Fix updating variables during variable imports
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33932:
URL: https://github.com/apache/airflow/pull/33932#issuecomment-1700631854
I think we should a flag specifying that. I think the flag should be something of (probably too long name):
```
--action-on-existing-key: override (default) skip fail
```
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1311122201
##########
airflow/www/views.py:
##########
@@ -5146,8 +5147,13 @@ def varimport(self):
flash("Missing file or syntax error.", "error")
return redirect(self.get_redirect())
else:
+ skipped = set()
suc_count = fail_count = 0
for k, v in variable_dict.items():
+ if session.scalar(select(models.Variable).where(models.Variable.key == k)):
+ logging.warning("Variable: %s already exist, skipping.", k)
Review Comment:
```suggestion
logging.warning("Variable: %s already exists, skipping.", k)
```
--
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] ephraimbuddy commented on a diff in pull request #33932: Fix updating variables during variable imports
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1311701311
##########
airflow/www/views.py:
##########
@@ -5146,8 +5147,13 @@ def varimport(self):
flash("Missing file or syntax error.", "error")
return redirect(self.get_redirect())
else:
+ skipped = set()
suc_count = fail_count = 0
for k, v in variable_dict.items():
+ if session.scalar(select(models.Variable).where(models.Variable.key == k)):
Review Comment:
Seems like this will be better since we are not querying a lot of values
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1311123249
##########
airflow/www/views.py:
##########
@@ -5146,8 +5147,13 @@ def varimport(self):
flash("Missing file or syntax error.", "error")
return redirect(self.get_redirect())
else:
+ skipped = set()
suc_count = fail_count = 0
for k, v in variable_dict.items():
+ if session.scalar(select(models.Variable).where(models.Variable.key == k)):
Review Comment:
This can use `exists_query` instead
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312799856
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,18 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
+ if action_on_existing == "fail":
+ existing_key = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
+ if existing_key:
+ raise SystemExit(f"Failed. These keys: {list(sorted(existing_key))} already exists.")
+
for k, v in var_json.items():
+ if action_on_existing == "skip":
+ if session.scalar(select(Variable).where(Variable.key == k)):
+ skipped.add(k)
+ continue
Review Comment:
We can do
```python
if action_on_existing == "overwrite":
existing_key = set()
else:
...
```
This makes the message at the very end easier as well.
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312669463
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,16 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
for k, v in var_json.items():
+ if action_on_existing != "overwrite":
+ exists_q = session.scalar(select(Variable).where(Variable.key == k))
Review Comment:
This could be a bit expensive. Something like this could be better
```python
existing_ids = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
if existing_ids and action_on_existing == "fail":
# Fail, existing variable keys found.
```
--
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] ephraimbuddy commented on pull request #33932: Fix updating variables during variable imports
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on PR #33932:
URL: https://github.com/apache/airflow/pull/33932#issuecomment-1701136650
> I think we should a flag specifying that behaviour. I think the flag should be something of (probably too long name):
>
> ```
> --action-on-existing-key: override (default) skip fail
> ```
>
> IMHO override shoudl remain as it is (default), as it could break existing usages.
Implemented this also in the UI with radios:
I can't think of a better name than the one you suggested.
--
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 #33932: Fix updating variables during variable imports
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33932:
URL: https://github.com/apache/airflow/pull/33932#discussion_r1312790021
##########
airflow/cli/commands/variable_command.py:
##########
@@ -86,7 +87,18 @@ def variables_import(args):
except JSONDecodeError:
raise SystemExit("Invalid variables file.")
suc_count = fail_count = 0
+ skipped = set()
+ action_on_existing = args.action_on_existing_key
+ if action_on_existing == "fail":
+ existing_key = set(session.scalars(select(Variable.key).where(Variable.key.in_(var_json))))
+ if existing_key:
+ raise SystemExit(f"Failed. These keys: {list(sorted(existing_key))} already exists.")
+
for k, v in var_json.items():
+ if action_on_existing == "skip":
+ if session.scalar(select(Variable).where(Variable.key == k)):
+ skipped.add(k)
+ continue
Review Comment:
If we have `existing_key` here, we can do
```python
for k, v in var_json.items():
if action_on_existing == "skip" and k in existing_key:
continue
Variable.set(...)
```
and save the query here.
--
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