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

Remove MergedConfig #457

Merged
merged 8 commits into from
Jan 22, 2024
Merged

Remove MergedConfig #457

merged 8 commits into from
Jan 22, 2024

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Jan 18, 2024

Description

Fixes #456.

This is intended to solve two things:

  1. Having to add every new config field to umpteen places, adding a lot of busywork to every change (and a lot of annoying indirection when trying to chase down the real definition of everything).
  2. MergedConfig containing the union of all roles' options, making it unclear which fields available when (and the especially comical case where resources exists in all variants... but still has a separate accessor method for each).

Instead, this PR delegates actually-common options to the new struct CommonNodeConfig and introduces a new enum AnyNodeConfig which users can explicitly downcast from to access role-specific options. Common options are accessible immediately because AnyNodeConfig derefs to CommonNodeConfig.

This PR replaces all uses of &dyn MergedConfig with &AnyNodeConfig to minimize impact. In the future, it's intended that new code that is only relevant for a single role should use that role's config type directly.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

Reviewer

Preview Give feedback

Acceptance

Preview Give feedback

@nightkr nightkr requested a review from a team January 18, 2024 17:03
@razvan razvan requested review from razvan and removed request for a team January 22, 2024 08:18
razvan
razvan previously approved these changes Jan 22, 2024
Copy link
Member

@razvan razvan left a comment

Choose a reason for hiding this comment

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

nice. maybe an entry in the change log would be nice, even if it's not user facing.

@nightkr
Copy link
Member Author

nightkr commented Jan 22, 2024

I would argue that these externally invisible changes don't make sense to add, "refactored MergedConfig" isn't something that tells a user.. anything.

@razvan
Copy link
Member

razvan commented Jan 22, 2024

This is probably a discipline vs. common sense discussion. I can get behind both arguments, just mentioned it because it's a checkbox for the reviewer.

@nightkr nightkr added this pull request to the merge queue Jan 22, 2024
Merged via the queue into main with commit 4b47b2c Jan 22, 2024
30 checks passed
@nightkr nightkr deleted the refactor/untraitify-mergedconfig branch January 22, 2024 14:46
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.

Drop MergedConfig trait
2 participants