Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(google,log): Avoid log name overriding #38071

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

AlexisBRENON
Copy link
Contributor


Avoid the use of the generic name attribute which is overrode by the dict configurator.

I'm not sure if this change break the backward compatibility (name attribute is "public" and maybe someone do some things with it...).

@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch from 812ad29 to c2b9843 Compare March 18, 2024 14:14
@shahar1
Copy link
Contributor

shahar1 commented Mar 19, 2024

Please notice that static tests fail

@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch 2 times, most recently from 932c339 to 4b999dd Compare March 20, 2024 08:09
@AlexisBRENON
Copy link
Contributor Author

I updated the PR to print a warning when using the name constructor argument.
However, my test seems to not pass... https://github.com/apache/airflow/actions/runs/8355631792/job/22871292056?pr=38071
It asks for the default application credentials. But I tried to mock the "transport" stuff to avoid all the required authentication.

@shahar1
Copy link
Contributor

shahar1 commented Mar 20, 2024

I updated the PR to print a warning when using the name constructor argument. However, my test seems to not pass... https://github.com/apache/airflow/actions/runs/8355631792/job/22871292056?pr=38071 It asks for the default application credentials. But I tried to mock the "transport" stuff to avoid all the required authentication.

Try to copy similar logic from existing tests

@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch from 4b999dd to 632411b Compare March 21, 2024 09:59
@AlexisBRENON
Copy link
Contributor Author

Seems good. Failing tests don't seem to be linked to my code.

@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch 2 times, most recently from eb1eae8 to ce39fec Compare March 22, 2024 13:43
@eladkal eladkal added this to the Airflow 2.9.1 milestone Mar 22, 2024
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Mar 22, 2024
@potiuk
Copy link
Member

potiuk commented Mar 26, 2024

Errors + static checks need to be fixed.

@eladkal eladkal modified the milestones: Airflow 2.9.1, Airflow 2.9.0 Mar 26, 2024
@AlexisBRENON
Copy link
Contributor Author

AlexisBRENON commented Mar 27, 2024

Errors + static checks need to be fixed.

Static checks are about single line newsgragment. But the comment from @eladkal suggests to add more info than a single line. Is there a way to by-pass this check or should I put the explanation somewhere else ?

@potiuk
Copy link
Member

potiuk commented Mar 27, 2024

Yes. use significant newsfragment.

@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch from ce39fec to 2618e13 Compare March 27, 2024 13:00
@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch from 2618e13 to e81b66a Compare March 27, 2024 14:07
Avoid the use of the generic `name` attribute which is overrode by the
dict configurator.
@AlexisBRENON AlexisBRENON force-pushed the fix/stackdriver_task_log_handler branch from e81b66a to 3ec4dc0 Compare March 28, 2024 07:29
Copy link
Contributor

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

LGTM

@eladkal eladkal merged commit 091d5e6 into apache:main Apr 3, 2024
48 checks passed
ephraimbuddy pushed a commit that referenced this pull request Apr 29, 2024
Avoid the use of the generic `name` attribute which is overrode by the
dict configurator.

(cherry picked from commit 091d5e6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants