-
Notifications
You must be signed in to change notification settings - Fork 47
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
133-replace-sed-commands-with-salt-file-module #140
133-replace-sed-commands-with-salt-file-module #140
Conversation
Well howdy, @charles-boyd ! Thanks a ton for submitting this. The team is largely heads down on releasing SecureDrop 0.9.0 today, but I look forward to reviewing these changes this week. Will also have a PR filed shortly to address #139, which is currently blocking use of the full dev env for the Workstation. |
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.
Changes as presented fail due to lack of globally unique SLS task names. Left recommended format for further tweaking.
Thanks for submitting, @charles-boyd! Happy to give it another run in the dev env after changes are addressed.
sed -i '1isd-decrypt sd-svs allow' /etc/qubes-rpc/policy/qubes.OpenInVM: | ||
cmd.run: | ||
- unless: grep -qF 'sd-decrypt sd-svs allow' /etc/qubes-rpc/policy/qubes.OpenInVM | ||
/etc/qubes-rpc/policy/qubes.OpenInVM: |
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.
In Salt, top-level task titles, which are the top key of a YAML dict, must be globally unique across the Salt config. That means we cannot reuse the filename as the title, since we modify this same file for different reasons across the Salt config (as shown in the diff you're presenting here).
Let's instead default to a reasonable pragma for naming these types of task intuitively: <vm_name>-[dom0-]<filename>
. So in this case: sd-decrypt-dom0-qubes.OpenInVM
. The entire task would then look like:
sd-decrypt-dom0-qubes.OpenInVM:
file.line:
- name: /etc/qubes-rpc/policy/qubes.OpenInVM
- mode: insert
- location: start
Similar changes should be made to the other file.line
conversions presented here.
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.
Thanks for the review and thorough explanation, @conorsch! I will the PR with your suggested changes.
We built on this in #146 which just got merged, so I'm going to close this PR. Thanks again @charles-boyd ! |
This merge should resolve #133.