Conversation
Nemric
force-pushed
the
user-units
branch
2 times, most recently
from
f28a6b2 to
987794f
Compare
|
Hi, |
|
@Nemric - I restarted the |
|
I logged in and restarted it, but one easy way to restart CI if you don't have access to log in is to rebase your PR and re-push. |
|
The |
bgilbert
left a comment
bgilbert
left a comment
There was a problem hiding this comment.
Nice work. This is getting closer!
internal/exec/stages/files/units.go
Outdated
| if err := s.relabelPath(filepath.Join(s.DestDir, path)); err != nil { | ||
| return err | ||
| } | ||
| paths[path] = true |
There was a problem hiding this comment.
We don't use the false and true values. paths can be a map[string]struct{}.
There was a problem hiding this comment.
true or false are used here if _, value := paths[path]; !value {} to check if paths have been relabelled or not.
This prevent multiple relabel ... that could be in a separate PR as we said previously ... I'm thinking about it
There was a problem hiding this comment.
Nope, you're ignoring the map value (the first return value) and only checking whether the key is present (the second return value). So any map value will do, and traditionally struct{}{} is used in that case.
There was a problem hiding this comment.
I will try to manage multiple relabelling in a better way, in another PR, so this piece of code should disappear, it seems to be a better approach isn't it ?
internal/exec/stages/files/units.go
Outdated
| relabelPath := f.Node.Path[len(s.DestDir):] | ||
| if err := s.Logger.LogOp( | ||
| func() error { return s.PerformFetch(f) }, | ||
| "writing unit %q at %q", unit.Key(), f.Node.Path, |
There was a problem hiding this comment.
Key() is not meant to be human-readable.
| } | ||
| if !relabeledDropinDir { | ||
| s.relabel(filepath.Dir(relabelPath)) | ||
| relabeledDropinDir = true |
There was a problem hiding this comment.
This assumes FilesFromSystemdUnitDropin returns values in sorted order.
Overall I'm wondering if it'd be better to have a separate preparatory PR that teaches the relabeling infrastructure to avoid redundant relabeling, so the rest of the codebase doesn't need to think about it anymore. These individual checks are annoying to maintain.
There was a problem hiding this comment.
I agree, that's why I did it on a previous commit : #1303 (comment)
There was a problem hiding this comment.
Yep, you did. 🙂 Since it's a cleanup that isn't directly related to this PR, and it's not a tiny change (since it'll allow some existing code to be simplified), it'd be better to move that change to a separate PR that can land first.
| } | ||
| } | ||
|
|
||
| func TestCreateUnits(t *testing.T) { |
There was a problem hiding this comment.
We generally don't have unit tests that require root or that modify the filesystem; we use blackbox tests for that. This might be okay, but I think it's surprising to have it here.
There was a problem hiding this comment.
I did that because I work in a Golang container that can't (or at least I don know how) run blackbox test...
I was written in the same way as
On the other hand, It was used for debugging purpose :)
There was a problem hiding this comment.
Oh, I didn't know about that code. That's a different case though; it's testing some of the C bindings, not the general behavior of some Ignition feature.
During development, of course feel free to write code however is convenient for you. Let's get this cleaned up before merging, though.
There was a problem hiding this comment.
ok, will remove that before merging :)
Nemric
force-pushed
the
user-units
branch
2 times, most recently
from
7c0975f to
e9794b3
Compare
3 participants
This should allow users to create user-level systemd units in ignition
I've added tests that write units in internal/exec/stages/files/units_test.go