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

PresetDefinition.with_additional_config bugfix #3987

Merged

Conversation

esztermarton
Copy link
Contributor

Replacing self.run_config with {} pre merge_dicts if self.run_config is falsey

Summary

Bugfix.
Related ticket has all info on context: #3985

I found minimal change to be to add
initial_config = self.run_config or {}
This will also ensure that the new config is a new dict (rather than the same dict object that was passed in).

Happy for alternative suggestions.

Test Plan

Added happy path test to assert that even if initial run_config is None, the with_additional_config won't fail and that a new object will be returned and that it will have expected config.

Have considered also adding test case permutations where the config-to-be-added is a dict vs None - however there would be a problem with the assertion that the object is always a new object (which atm would not be the case if the new_runconfig is None). This test would align with docstring - but it would require more code changes and would also change current behaviour.

Checklist

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@vercel
Copy link

vercel bot commented Apr 1, 2021

Someone is attempting to deploy a commit to the Elementl Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Apr 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/elementl/dagster/973nekZG3VBBdbQh9Qmy2yu8Smny
✅ Preview: https://dagster-git-fork-esztermarton-allownooriginalconfig-elementl.vercel.app

@alangenfeld
Copy link
Member

thanks!

@alangenfeld alangenfeld merged commit be275ca into dagster-io:master Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants