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

broker: cleanup up attribute initialization code #5543

Merged
merged 3 commits into from
Nov 11, 2023

Conversation

garlick
Copy link
Member

@garlick garlick commented Nov 8, 2023

Problem: after #5536 was merged, the broker code for initializing "attributes from the environment" looks over-engineered.

Trim it down.
Also fix some man page typos that were adjacent to the work in #5536.

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -120,7 +120,7 @@ broker.rc1_path [Updates: C]
The path to the broker's rc1 script. Default: ``${prefix}/etc/flux/rc1``.

broker.rc3_path [Updates: C]
The path to the broker's rc3 script. Default: ``${prefix}/etc/flux/rc1``.
The path to the broker's rc3 script. Default: ``${prefix}/etc/flux/rc3``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in commit message: s/thes/the/

Problem: there are only two attributes to set from the broker
environment now, and the code is more complex than it need be.

Simplify code.
Problem: flux-config(1) has a cut and paste error in the blurb
on conf.shell_initrc.

Correct error.
Problem: flux-broker-attributes(7) has a cut and paste error in the
entry for broker.rc3_path.

Correct error.
Copy link

codecov bot commented Nov 11, 2023

Codecov Report

Merging #5543 (5b329ba) into master (8b2bdfb) will increase coverage by 28.10%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           master    #5543       +/-   ##
===========================================
+ Coverage   55.36%   83.47%   +28.10%     
===========================================
  Files         439      487       +48     
  Lines       74728    82299     +7571     
===========================================
+ Hits        41375    68697    +27322     
+ Misses      33353    13602    -19751     
Files Coverage Δ
src/broker/broker.c 78.95% <77.77%> (+20.30%) ⬆️

... and 410 files with indirect coverage changes

@mergify mergify bot merged commit 865647e into flux-framework:master Nov 11, 2023
32 checks passed
@garlick garlick deleted the broker_attr_cleanup branch November 11, 2023 13:39
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.

2 participants