You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "shubhamraj-git (via GitHub)" <gi...@apache.org> on 2024/10/17 15:28:39 UTC
[PR] Better handling masking of values of set variable [airflow]
shubhamraj-git opened a new pull request, #43123:
URL: https://github.com/apache/airflow/pull/43123
<!-- Please keep an empty line above the dashes. -->
Better handling masking of values of set variable
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.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
Re: [PR] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806056290
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
Review Comment:
Since this is very subjective to cli commands, Do we really need to take it out? I see the sub-commands mentioned here only for checking based upon flags (just the line above), to keep everything on same place, I wrote the variables subcommand here only and since this is specific to keys rather than flag, i used new variable.
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806484373
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
I was thinking index as it will be referred using that. Open to `sub_commands_to_check_for_sensitive_key` to.
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
I was thinking index as it will be referred using that. Open to `sub_commands_to_check_for_sensitive_key` too
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806059977
##########
tests/utils/test_cli_util.py:
##########
@@ -185,6 +185,47 @@ def test_get_dag_by_pickle(self, session, dag_maker):
with pytest.raises(AirflowException, match="pickle_id could not be found .* -42"):
get_dag_by_pickle(pickle_id=-42, session=session)
+ @pytest.mark.parametrize(
+ ["given_command", "expected_masked_command"],
+ [
+ (
+ "airflow variables set --description 'needed for dag 4' client_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set --description 'needed for dag 4' client_secret_234 ********",
+ ),
+ (
+ "airflow variables set cust_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set cust_secret_234 ********",
Review Comment:
This is different test case, I am pointing too. I have included both type of cli commands which users can use to set variables, and the masking should be done on both types.
--
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] Better handling masking of values of set variable [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1811395460
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
Yeah. Also it's good for the future, when we will separate API package (after 3.0 likely) -this is way more "standalone" and simple.
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1805988019
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
`airflow variables import` has a 4th parameter, so does many other commands, they have lot of extra options. So better to check that its a `airflow variables set` 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
Re: [PR] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806057916
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
This is for keys,
We are checking the keys for the value to be masked. If the key contains some sensitive word, mask the value.
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806405601
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
Understandable. The variable naming isn't too clear.
Instead if we refactor `sub_commands_to_check` as `sub_commands_to_check_for_sensitive_fields` and this one (`sub_commands_to_check_based_upon_key`) as `sub_commands_to_check_for_sensitive_indexes` would make more semantic sense
##########
tests/utils/test_cli_util.py:
##########
@@ -185,6 +185,47 @@ def test_get_dag_by_pickle(self, session, dag_maker):
with pytest.raises(AirflowException, match="pickle_id could not be found .* -42"):
get_dag_by_pickle(pickle_id=-42, session=session)
+ @pytest.mark.parametrize(
+ ["given_command", "expected_masked_command"],
+ [
+ (
+ "airflow variables set --description 'needed for dag 4' client_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set --description 'needed for dag 4' client_secret_234 ********",
+ ),
+ (
+ "airflow variables set cust_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set cust_secret_234 ********",
Review Comment:
My bad.
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
Review Comment:
Answered above, this can be ignored and answered in the other comment as that will comprise this 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
Re: [PR] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #43123:
URL: https://github.com/apache/airflow/pull/43123#issuecomment-2422473196
Test failures look unrelated, but the static checks needs fixing.
@shubhamraj-git you need to set up precommit and use it for your commits. Check: https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst
--
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] Better handling masking of values of set variable [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1811401791
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
Yep. Good choices.
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1811237658
##########
airflow/utils/cli.py:
##########
@@ -139,11 +140,18 @@ def _build_metrics(func_name, namespace):
:param namespace: Namespace instance from argparse
:return: dict with metrics
"""
- sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_for_sensitive_fields = {"users", "connections"}
+ sub_commands_to_check_for_sensitive_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
Review Comment:
nit: for cases when values under `sub_commands_to_check_for_sensitive_key` have sensitive info
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
Right, that makes sense since we only use it in audit endpoint / ui.
--
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] Better handling masking of values of set variable [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #43123:
URL: https://github.com/apache/airflow/pull/43123
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806426162
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
I think this is under wrapper and used before action logging, we need to change the value at this moment only. If my understanding is right, they have implemented same design for -p (password) instead of using the mask_secret and saving it's pattern, they changed here itself, since this itself will be logged.
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806426162
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
I think this is under wrapper and used before action logging, we need to change the value at this moment only. They have implemented same design for -p (password) instead of using the mask_secret and saving it's pattern, they changed here itself, since this itself will be logged.
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806432713
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
@amoghrajesh this makes sense.
`sub_commands_to_check` as `sub_commands_to_check_for_sensitive_fields`
Also,
Should we better do `sub_commands_to_check_based_upon_key` to `sub_commands_to_check_for_sensitive_key` since index would be bit in different context?
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1811246643
##########
airflow/utils/cli.py:
##########
@@ -139,11 +140,18 @@ def _build_metrics(func_name, namespace):
:param namespace: Namespace instance from argparse
:return: dict with metrics
"""
- sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_for_sensitive_fields = {"users", "connections"}
+ sub_commands_to_check_for_sensitive_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
Review Comment:
Sure
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806402303
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
Wondering it using `mask_secret` would instead do a better job here, it would hide if by adding it as a secret mask and every further occurence will be taken care of.
cc @potiuk
--
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] Better handling masking of values of set variable [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1805984111
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
Review Comment:
Add a new structure called sensitive indexes and define that instead of hardcoding here
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
This is the full command right:
```
"full_command": "['/usr/local/bin/airflow', 'variables', 'set', '--description', 'my-secret', 'client_secret_2149', '********']"}
```
So we need to get -1 of it since -1 would always be "value".
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
Let us add "variables" here
##########
tests/utils/test_cli_util.py:
##########
@@ -185,6 +185,47 @@ def test_get_dag_by_pickle(self, session, dag_maker):
with pytest.raises(AirflowException, match="pickle_id could not be found .* -42"):
get_dag_by_pickle(pickle_id=-42, session=session)
+ @pytest.mark.parametrize(
+ ["given_command", "expected_masked_command"],
+ [
+ (
+ "airflow variables set --description 'needed for dag 4' client_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set --description 'needed for dag 4' client_secret_234 ********",
+ ),
+ (
+ "airflow variables set cust_secret_234 7fh4375f5gy353wdf",
+ "airflow variables set cust_secret_234 ********",
Review Comment:
Let us add the description field too, otherwise it gives an intuition that masking removes description.
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
- if sub_command in sub_commands_to_check:
+ # For cases when value in variables have sensitive value
+ if sub_command in sub_commands_to_check_based_upon_key:
+ key = full_command[-2] if len(full_command) > 3 else None
Review Comment:
`airflow variables import` has a 4th parameter, so does many other commands, they have lot of extra options. So better to check that its a `airflow variables set` 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
Re: [PR] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806062976
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
Review Comment:
Ideally, I have bifurcated both, since these check based upon the flags, and the variables need based upon the keys which is positional args. Adding both at the same place would be wrong, since both need different operations.
--
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] Better handling masking of values of set variable [airflow]
Posted by "shubhamraj-git (via GitHub)" <gi...@apache.org>.
shubhamraj-git commented on code in PR #43123:
URL: https://github.com/apache/airflow/pull/43123#discussion_r1806056290
##########
airflow/utils/cli.py:
##########
@@ -140,10 +141,17 @@ def _build_metrics(func_name, namespace):
:return: dict with metrics
"""
sub_commands_to_check = {"users", "connections"}
+ sub_commands_to_check_based_upon_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
Review Comment:
Are you talking about `sensitive_fields` ? If yes, it is already there, touching it might need to make more changes. and also this is respective to cli commands.
--
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