Skip to content

Conversation

@khalidmammadov
Copy link
Contributor

Current postgres password gets masked during DB engine init and added to secret list and masked whenever mentioned in the logs.
As it currently set to "airflow" it's converted to *** everywhere airflow is printed in the logs including folder path etc.

Currently, this will fix one of the failing test cases from quarantined list that expects "airflow" path in the log but gets ***.
But this also should be noted for future similar cases as to not to name password as "airflow" or similar reserved/widely used words as they will be masked in the logs.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@mik-laj
Copy link
Member

mik-laj commented Oct 21, 2021

this will fix one of the failing test cases from quarantined list

Do you remember which one?

@khalidmammadov
Copy link
Contributor Author

This one: tests.cli.commands.test_task_command.TestLogsfromTaskRunCommand
It asserts for this value:
"INFO - Running: ['airflow', 'tasks', 'run'...."
but gets
"INFO - Running: ['***', 'tasks', 'run'..."

@potiuk
Copy link
Member

potiuk commented Oct 22, 2021

Should we unquarantine the quarantined test as well ?

@khalidmammadov
Copy link
Contributor Author

@potiuk This PR tries to fix only 1 of 5 failing test cases within the suite and whole suite is marked as @pytest.mark.quarantined.
I didnt have chance to take a look to others so I think we can leave quarantined marker until others are also fixed.

That aside, although I am working on getting password changed everywhere it's used (which is quite a lot) I am not sure if it's ok from community perspective to do it and if they would prefer to keep as is i.e. "airflow"? In which case masking replaces airflow word with *** everywhere in the logs which will be problem I think all the time

@potiuk
Copy link
Member

potiuk commented Oct 22, 2021

I see. Makes sense. I think it makes sense to change the password if it is in unit tests

@khalidmammadov khalidmammadov force-pushed the fix_quarantined_test_postgres_password_issue branch from 55e9955 to d929db2 Compare October 22, 2021 21:24
@khalidmammadov khalidmammadov force-pushed the fix_quarantined_test_postgres_password_issue branch 5 times, most recently from 22e22ad to d933cb8 Compare November 8, 2021 14:02
@potiuk potiuk added the debug ci resources Set it on PR if you want to debug resource usage for it label Nov 9, 2021
@potiuk potiuk closed this Nov 9, 2021
@potiuk potiuk reopened this Nov 9, 2021
@khalidmammadov khalidmammadov force-pushed the fix_quarantined_test_postgres_password_issue branch 2 times, most recently from 4063dbc to 75cc13c Compare November 11, 2021 15:02
@khalidmammadov khalidmammadov force-pushed the fix_quarantined_test_postgres_password_issue branch from 75cc13c to b55477e Compare November 12, 2021 09:39
@khalidmammadov
Copy link
Contributor Author

Closing in favor of this: #19858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools debug ci resources Set it on PR if you want to debug resource usage for it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants