Skip to content

fix(config): remove deny_unknown_fields#4503

Closed
lucasfernog wants to merge 2 commits intodevfrom
fix/config-updates
Closed

fix(config): remove deny_unknown_fields#4503
lucasfernog wants to merge 2 commits intodevfrom
fix/config-updates

Conversation

@lucasfernog
Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@lucasfernog lucasfernog requested a review from a team as a code owner June 28, 2022 13:11
@FabianLars
Copy link
Copy Markdown
Member

what do you think about logging unknown fields in the future (only on values actually specified in the config if possible)?

@lucasfernog
Copy link
Copy Markdown
Member Author

The CLI validates the schema so we're not left in the dark here. Idk if it can be improved.

@FabianLars
Copy link
Copy Markdown
Member

Yeah, i was thinking about what if users see new features/config options and only update the cli and wonder why the config doesn't have any effect

@lucasfernog
Copy link
Copy Markdown
Member Author

I don't think there's an easy way to do that using serde. Maybe we should just improve the error message instead.

@lucasfernog
Copy link
Copy Markdown
Member Author

I think I'll change the CLI to pass only the merge config instead to avoid conflicts.

@lucasfernog
Copy link
Copy Markdown
Member Author

Closing in favor of #4598

@lucasfernog lucasfernog closed this Jul 5, 2022
@lucasfernog lucasfernog deleted the fix/config-updates branch July 5, 2022 23:09
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.

2 participants