Conversation
salt/sys-git/create-sync.sls
Outdated
| - {{ slsdotpath }}.clone | ||
|
|
||
| {# TODO: make this properly read configuration from pillar instead of inlined in code #} | ||
| {# % set syncs = [ |
There was a problem hiding this comment.
This is a new pattern to the qusal repo and a simple solution to the "allowing user-configured instantiation of multiple appvm instances per state" question. I have locally been refactoring some of the other modules to use the same structure in order to declaratively define my appvms in code and it works all right in general.
Alternatives considered:
- Declare them in the topfile
- This would be the nicest IMO. While we can specify minions in topfile, there is not any interface to pass values/vars via it AFAIK. Therefore it would involve also complementing with values from pillars/grains.
- Keep the general
foreachincreate.slsthe same as here but populatesyncsentirely from pillar- This seems ideal. While I have been making use of the
dotfilespillar, I was not successful in having the configuration here picked up. Not sure if complex values simply silently fail or it's a configuration issue with my local environment.
- This seems ideal. While I have been making use of the
- Runtime switching on grains
- Not a fan of this approach
There was a problem hiding this comment.
Please discuss about it on #74, it makes sense to do such thing at a larger scale that impacts all formulas and therefore needs more thought. Would be better if it worked with pillars.
Would make sense to remove this file until #74 is completed to not have inconsistencies such as only this state handling pillars and others not, so this PR can be merged.
There was a problem hiding this comment.
Removed create-sync.sls, create-sync.top and updated readme accordingly.
ben-grande
left a comment
ben-grande
left a comment
There was a problem hiding this comment.
Have you seen qusal.ConnectTCP? It is what I am using for proxying connections from dev (not netvm) to a git server/forge.
If I were to use sys-git-sync, it would not have a netvm and users would need to configure access via qusal.ConnectTCP to their forges.
Instead of fixing the errors showed by the review, please focus on argumentation of the value of git-sync. Things I see:
- No connectivity on the development environment (great), but now every operations requires to be repeated on both
devqubesys-git-sync,push,fetchetc (not great). - Reviewing git patches are more difficult than reviewing pure text (mail).
salt/sys-git/create-sync.sls
Outdated
| - {{ slsdotpath }}.clone | ||
|
|
||
| {# TODO: make this properly read configuration from pillar instead of inlined in code #} | ||
| {# % set syncs = [ |
There was a problem hiding this comment.
Please discuss about it on #74, it makes sense to do such thing at a larger scale that impacts all formulas and therefore needs more thought. Would be better if it worked with pillars.
Would make sense to remove this file until #74 is completed to not have inconsistencies such as only this state handling pillars and others not, so this PR can be merged.
| - qvm: {{ slsdotpath }} | ||
|
|
||
| {% load_yaml as defaults -%} | ||
| name: {{ slsdotpath }}-sync |
There was a problem hiding this comment.
The tag git-sync seems to be missing.
There was a problem hiding this comment.
Hm, the template itself shouldn't have the tag should be applied to the AppVMs directly I think? If the tag is used in qubes-rpc policies it would typically be unintended to reference the template I think?
salt/sys-git/README.md
Outdated
|
|
||
| git remote add ghost https://github.com/ghost/qubes-doc | ||
| git fetch ghost master | ||
| # inspect changes |
There was a problem hiding this comment.
Different than e-mails, git patches can be quite huge depending on the commit and how many of them exist. It works at a small scale (such as reading e-mails), but on a larger scale, requires checking the patch is the intended one or not.
salt/sys-git/README.md
Outdated
| # inspect changes | ||
|
|
||
| # optionally, resign commit with pgp if you have split-gpg2 set up | ||
| git commit -S --amend |
There was a problem hiding this comment.
If you give PGP signing access to git-sync which is networked, it lowers its security.
There was a problem hiding this comment.
Arguable tradeoff. Anyway, dropped this instruction
salt/sys-git/README.md
Outdated
|
|
||
| git remote add ghost https://github.com/ghost/qubes-doc | ||
| git fetch ghost master | ||
| # inspect changes |
There was a problem hiding this comment.
I prefer to not have inline comments in markdown except on rare occasions.
There was a problem hiding this comment.
Broke up the block and removed inline markdown comment
2 participants
Contribution checklist
Before contributing, I, the opener of this request:
GitHub Web; and
Lacking to check any of the above can result in the rejection of the
contribution without no further comment.
What does this PR aims to accomplish
With a similar philosophy as the splittMutt approach in
mail, this introduces separate role and qube(s) for push/pull operation with remote repos, to be done separately from eithersys-gititself ordevetc.What does this PR change
Add new states
sys-git.create-syncandsys-git.install-sync.