-
Notifications
You must be signed in to change notification settings - Fork 104
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
[develop] Reuse the rotation script inside the munge resource #2471
Conversation
…e resource when retrieving the custom munge key from Secrets Manager
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2471 +/- ##
========================================
Coverage 76.06% 76.06%
========================================
Files 13 13
Lines 1876 1876
========================================
Hits 1427 1427
Misses 449 449
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Good job. Just implement the change I requested and then we can merge it.
cookbooks/aws-parallelcluster-slurm/resources/munge_key_manager.rb
Outdated
Show resolved
Hide resolved
…rotation script to make it can be used by config and update process.
…ned method dirname for Chef::Resource::File:Class
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.
This is not what we had discussed yesterday. Please address the comments.
cookbooks/aws-parallelcluster-slurm/templates/default/slurm/head_node/update_munge_key.sh.erb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/templates/default/slurm/head_node/update_munge_key.sh.erb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/templates/default/slurm/head_node/update_munge_key.sh.erb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/templates/default/slurm/head_node/update_munge_key.sh.erb
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/recipes/config/config_head_node.rb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/resources/munge_key_manager.rb
Outdated
Show resolved
Hide resolved
cookbooks/aws-parallelcluster-slurm/resources/munge_key_manager.rb
Outdated
Show resolved
Hide resolved
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.
You have to rebase your PR on top of other developments, so I will just provide a comment here.
cookbooks/aws-parallelcluster-slurm/recipes/config/config_head_node.rb
Outdated
Show resolved
Hide resolved
user 'root' | ||
group 'root' | ||
cwd ::File.dirname(script_path) | ||
command "./#{::File.basename(script_path)} -d" | ||
end | ||
end |
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 believe you can simplify this block:
def fetch_and_decode_munge_key
declare_resource(:execute, 'fetch_and_decode_munge_key') do
user 'root'
group 'root'
command "/#{node['cluster']['scripts_dir']}/slurm/update_munge_key.sh -d"
end
end
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.
Great comment! Thank you Jacopo.
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 wonder if we actually need the first /
character in the command
variable. Though, in principle scripts_dir
will always be an absolute path, so it shouldn't hurt.
cookbooks/aws-parallelcluster-slurm/templates/default/slurm/head_node/update_munge_key.sh.erb
Show resolved
Hide resolved
…fig_head_node_directories recipe
user 'root' | ||
group 'root' | ||
cwd ::File.dirname(script_path) | ||
command "./#{::File.basename(script_path)} -d" | ||
end | ||
end |
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 wonder if we actually need the first /
character in the command
variable. Though, in principle scripts_dir
will always be an absolute path, so it shouldn't hurt.
Description of changes
Checklist
develop
add the branch name as prefix in the PR title (e.g.[release-3.6]
).