Skip to content

Merge lists from configuration and CLI #1721

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

Merged
merged 1 commit into from
Jun 17, 2021

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Jun 15, 2021

📔 Description

Before this change, we merged configuration from different sources in a way that instead of merging nested lists, we overwrote them—leading to plugins specified in a configuration file not to load at all when plugins were specified on the command line as well. Now, we merge the lists in order of precedence.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

File-by-file. Test locally.

Note that changelog entry + docs are yet to be written because I'd like to discuss this first with @tenzir/backend.

@dominiklohmann dominiklohmann added the bug Incorrect behavior label Jun 15, 2021
@dominiklohmann dominiklohmann requested a review from a team June 15, 2021 12:15
@dominiklohmann dominiklohmann force-pushed the story/ch26466/merge-order branch 4 times, most recently from c915a62 to 72a85eb Compare June 16, 2021 13:13
Before this change, we merged configuration from different sources in a
way that instead of merging nested lists, we overwrote them—leading to
plugins specified in a configuration file not to load at all when
plugins were specified on the command line as well. Now, we merge the
lists in order of precedence.
@dominiklohmann dominiklohmann force-pushed the story/ch26466/merge-order branch from 72a85eb to b1dd1d5 Compare June 16, 2021 17:02
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

I didn't look too deeply since this was already reviewed in a depth in a call with @tobim , but from what I can see the code is correct and the review comments were addressed.

@dominiklohmann dominiklohmann merged commit ccb3953 into master Jun 17, 2021
@dominiklohmann dominiklohmann deleted the story/ch26466/merge-order branch June 17, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants