-
Notifications
You must be signed in to change notification settings - Fork 12
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
[DPE-2198] Integrate sysctl lib #118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good, there is only one issue with charm config, which should be removed if anything failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass.
I have two major points:
- I would try to re-write slightly the business logic for
update
to make validation in memory and keep the status as clean as possible. I have a proposal in the comments that I believe would make things more robust. - I believe it would be best to encode in the library the removal of charm_filepath (and possibly creation, even with just empty content, just to make sure it is written in the header of the merged sysctl file). For doing this we should add some handlers to _on_install and _on_remove events (such that this is done by default and charm authors do not forget)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for cooperation.
Integrate sysctl lib into Kafka charm.
Proposed "default" values for os config are: