-
Notifications
You must be signed in to change notification settings - Fork 23
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
add script for symlinking paths in $EPREFIX to corresponding paths in host root #34
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.
Looks okay to me, though I was working on something similar with Ansible and I would prefer to have it there: I think that's cleaner, and it keeps all the configuration for the compatibility layer in a single file.
prefix-symlink-host-paths.sh
Outdated
ec=$? | ||
if [ $ec -ne 0 ]; then | ||
echo_yellow ">> [CHANGE] ${EPREFIX}$path is *not* a symlink to $path, fixing that..." | ||
rm ${EPREFIX}$path |
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.
If we want to do the same for directories (as CC suggested), this will have to be a bit more aggressive ;-)
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.
Sure, but since it's only files right now, I'd keep it at that for now.
That's not entirely true, since the set of packages to install is already in a separate file too... There's value in being able to run the steps "manually" as well outside of Ansible imho? |
True, but that file had to be part of the overlay repo. If I would use the Ansible playbook for installing the compatibility layer, I would prefer to have it all together (as much as possible), instead of having to edit both my variables file and bash scripts. I do see the added value of being able to run things manually, though. And since we already describe a manual procedure in the README anyway, maybe we should split it all up and make two different dirs with Ansible and bash stuff? |
@bedroge Splitting things up makes sense to me... |
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.
Could you also update .github/workflows/ansible-lint.yml and change line 26:
targets: "playbooks/*.yml"
to:
targets: "ansible/playbooks/*.yml"
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
rdma-core 34.0
cfr. #15