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

follow-up: config/config_store_test: ignored_keys test #15765

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented Dec 19, 2023

This test is mainly concerned with showing how to use config_store::read_yaml and how to ignore unknown keys that need to be interpreted by a separate config_store

related to #15759
followup for #15605

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

@vbotbuildovich
Copy link
Collaborator

Copy link
Contributor

@andrwng andrwng left a comment

Choose a reason for hiding this comment

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

Looks fine, I'm wondering if it makes sense to extend this a bit

Comment on lines 549 to 551
// actual test: check that noop_cfg can ignore properties for test_config
auto noop_cfg = noop_config{};
BOOST_REQUIRE_NO_THROW(noop_cfg.read_yaml(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it also make sense to read_yaml() into a config that has aliased_bool_legay=True and check that it is ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah but that's not how it works: the parameter is called ignored_missing: if the yaml key is not found in the properties of the caller, then it will throw std::illegal_argument unless it's in ignore_missing.

I'll add an extension to show this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, much clearer now! Thank you!

@andijcr andijcr force-pushed the followup/config_store_property_name_and_aliases branch from 2cb3359 to 6e051f0 Compare December 20, 2023 10:15
@andijcr
Copy link
Contributor Author

andijcr commented Dec 20, 2023

force push: rewrote the test to show how ignore_missing parameter work and should be used

@andijcr andijcr requested a review from andrwng December 20, 2023 10:17
this test is mainly concerned with showing how to use
config_store::read_yaml and how to ignore unknown keys
that needs to be intepreted by a separate config_store

related to issues/15759
@andijcr andijcr force-pushed the followup/config_store_property_name_and_aliases branch from 6e051f0 to b33ece9 Compare December 20, 2023 10:21
@andijcr andijcr merged commit d6559e7 into redpanda-data:dev Jan 4, 2024
19 checks passed
@andijcr andijcr deleted the followup/config_store_property_name_and_aliases branch January 4, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants