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

Updates to Config file Auto-generation (includes #138) #209

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

maurermi
Copy link
Collaborator

This PR looks to help complete #138 by making the changes requested in the most recent review. Namely, this change redirects the config file output to stdout, and confines all other other messages to stderr. In addition, adds changes which reflect current naming conventions and design choices.

@HalosGhost
Copy link
Collaborator

@maurermi note the merge conflict in CMakeLists.txt

@maurermi
Copy link
Collaborator Author

Merge resolved, updates required to unit tests to reflect new functionality as well (coming soon)

@maurermi maurermi force-pushed the config_generation branch 13 times, most recently from f6d52cf to 44ab494 Compare February 21, 2023 18:41
@maurermi maurermi marked this pull request as draft February 21, 2023 21:12
@maurermi maurermi force-pushed the config_generation branch 4 times, most recently from 65afd30 to 896e23a Compare February 22, 2023 19:04
= get_param_from_template_file(watchtower_count_key,
config_map);
return_msg += create_component(shard_count_key,
std::get<size_t>(shard_count),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If shard_count is not specified in the template file, or is initialized to anything besides a legal size_t value, std::get<size_t>(shard_count) will fail and cause an abort. This also applies to all below calls of create_component with their corresponding component names.

Since this is a tool external to the system, this is probably not a big deal, but I am putting this here for consideration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm also not too concerned about this. Though, anything that's a bit error-prone like this underscores that everything here would be less error-prone if we had better error-handling for config file parsing.

@maurermi maurermi marked this pull request as ready for review February 22, 2023 19:37
@maurermi
Copy link
Collaborator Author

@HalosGhost this is ready for a review -- in particular it would be good to know whether (and if so, what) additional functionality would be helpful to be included.

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

Looks like there are a couple of things to address (a few of them minor clean-ups or considerations, but one of them is definitely more pressing).

= get_param_from_template_file(watchtower_count_key,
config_map);
return_msg += create_component(shard_count_key,
std::get<size_t>(shard_count),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm also not too concerned about this. Though, anything that's a bit error-prone like this underscores that everything here would be less error-prone if we had better error-handling for config file parsing.

tools/config_generator/config_generator.cpp Outdated Show resolved Hide resolved
2pc-compose.cfg Outdated Show resolved Hide resolved
Removes identification of build directory, as sending file to
build directory has been replaced by priting config file to
stdout. Also adds support for seeding by generating a
seed key if necessary.

Update to properly print out config file to std error,
and merge conventions such that m_new_config is used
to store config file data

Signed-off-by: Michael Maurer <[email protected]>
@maurermi maurermi force-pushed the config_generation branch 2 times, most recently from af9767c to 2833dce Compare April 3, 2023 19:59
@HalosGhost HalosGhost self-requested a review April 3, 2023 20:26
Also updates config generation test to reflect previous behavior,
would be good to review if this behavior is still appropriate however.

Signed-off-by: Michael Maurer <[email protected]>
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