Skip to content

Conversation

@khalidmammadov
Copy link
Contributor

This is code improvement PR to remove a duplicate line. The code copies config in either cases of IF statement hence duplicate call.
Moving that before IF makes it unique and sufficient for the purpose.

I have moved first call above IF and removed ELSE block


^ 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.

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Oct 4, 2021
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 5, 2021
@khalidmammadov khalidmammadov force-pushed the remove_duplicate_code_call branch from dd9401c to febba29 Compare October 5, 2021 10:17
XD-DENG
XD-DENG previously requested changes Oct 5, 2021
Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Hi @khalidmammadov , I assume you were trying to rebase but having some issues?

Given there is one earlier approval, to block accidental merging, I'll add this change request to block merging.

@khalidmammadov khalidmammadov force-pushed the remove_duplicate_code_call branch from f70d729 to febba29 Compare October 5, 2021 12:49
@khalidmammadov
Copy link
Contributor Author

That's fine, thanks. I will let you know once I resolve the issue

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

These lines shouldn'be be duplicated -- check out 670f2ed when it was first added (they got broken a bit in the past by 151934c#diff-e039f5fcb6c655f83aef48e7d7e0c430224c3e1af92d150a6782a3d8baff13ef)

@khalidmammadov
Copy link
Contributor Author

@ashb Exactly, that's why I though to remove one of them.

Looks like there is an issue with the build pipeline that affects this as well. Something to do with azure-cosmos package version. Not sure who needs to look into it

@ashb
Copy link
Member

ashb commented Oct 5, 2021

@khalidmammadov Could you add the difference back instead please? :)

@khalidmammadov
Copy link
Contributor Author

khalidmammadov commented Oct 5, 2021

@ashb You mean add params to this tmp_configuration_copy function and restore below calls?
cfg_path = tmp_configuration_copy(chmod=0o600,
include_env=True,
include_cmds=True)

@khalidmammadov khalidmammadov force-pushed the remove_duplicate_code_call branch from febba29 to 91d9757 Compare October 5, 2021 14:29
@khalidmammadov
Copy link
Contributor Author

@ashb I did look into your references and as I see that function had 2 extra parameters which were not used in the function body when they were first introduced, although calls made some distinction by supplying different arguments which made no difference as they are not used! The second PR removed those 2 parameters as they have no value and was probably causing pylint to complain as that PR was for pylint integration.
So, this PR finally removes that second call for that function which was unnecessary from outset.

@khalidmammadov
Copy link
Contributor Author

@XD-DENG can you unlock it please, so I can see if builds are passing as I did one more rebase after that?

@XD-DENG XD-DENG dismissed their stale review October 5, 2021 16:28

Dismiss the blocking (there is another change request now anyway)

@potiuk potiuk closed this Oct 5, 2021
@potiuk potiuk reopened this Oct 5, 2021
@potiuk
Copy link
Member

potiuk commented Oct 5, 2021

Github Actions had outage: closed/reopended to rebuild.

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@khalidmammadov khalidmammadov requested a review from ashb October 6, 2021 08:18
@khalidmammadov
Copy link
Contributor Author

Closing this as @ashb solved the actual issue with the call and hence not duplicate anymore. #18772

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

Labels

area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants