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

ExchangeFetchAssignment deleting the wrong config #1315

Merged
merged 1 commit into from
May 11, 2020

Conversation

lzach
Copy link
Contributor

@lzach lzach commented Feb 10, 2020

The logic for displaying warning and updating config when a user supplied configuration using the old ExchangeFetch class has a couple of errors in it.

These errors mean that the ExchangeFetchAssignment ignores all configuration passed to it, if there's configurations for the ExchangeFetch class in the config file.

This is especially bad if someone has partially updated an old config file and accidentally left some references to ExchangeFetch in it, while correctly updating other references.

(The ExchangeReleaseAssignment is correct as it is.)

…lied configuration using the old ExchangeFetch class has a couple of errors in it.

These errors mean that the ExchangeFetchAssignment ignores all configuration passed to it, if there's configurations for the ExchangeFetch class in the config file.

(The ExchangeReleaseAssignment is correct as it is.)
@BertR BertR requested a review from jhamrick February 10, 2020 21:13
@BertR BertR added the bugfix label Feb 10, 2020
@willingc willingc requested review from willingc and BertR May 11, 2020 13:50
Copy link
Member

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Thanks @lzach for submitting this PR. Sorry we have been slow in responding.

The change looks good to me. I'll leave this open for a couple of days to see if @BertR would like to review as well.

Copy link
Contributor

@BertR BertR left a comment

Choose a reason for hiding this comment

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

LGTM!

@willingc willingc merged commit bee19dc into jupyter:master May 11, 2020
@BertR BertR added this to the 0.7.0 milestone Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants