Split templating from file IO operations #45
Conversation
atanasdinov
force-pushed
the
file-perm
branch
from
2bd2193
to
e7ba299
Compare
|
This is interesting because you trade programmatic convenience for pure decoupling. Admittedly, part of why I say that is because I still really don't like Go error handling, so having to recreate the two-step "apply template, write file" and handle the error each time feels cumbersome for the amount of components we're going to have that use that model on. |
|
I'm not sure if I'll be able to pull it off (since we have so many moving pieces) but my goal is try and decouple the file handling as much as possible from the rest of the business logic. This way we should be able to confirm all modifications to scripts and templates are properly executed (with the necessary tests in place) and only then write these results to the FS (regardless if this is the |
| @@ -10,7 +10,6 @@ import ( | |||
|
|
|||
| const ( | |||
| scriptsDir = "scripts" | |||
| scriptMode = 0o744 | |||
There was a problem hiding this comment.
Glad you addressed this now. After the user stuff landed (where I ended up having a separate constant for the perms in that file) I was going to revisit this.
pkg/build/raw_test.go
Outdated
| @@ -70,7 +70,7 @@ func TestWriteModifyScript(t *testing.T) { | |||
|
|
|||
| stats, err := os.Stat(expectedFilename) | |||
| require.NoError(t, err) | |||
| assert.Equal(t, fs.FileMode(modifyScriptMode), stats.Mode()) | |||
| assert.Equal(t, fs.FileMode(0o744), stats.Mode()) | |||
There was a problem hiding this comment.
I don't think hardcoding the permissions is the right move here. What the test is ensuring is that the correct variable was used when it was run, so IMO we should be testing that the right variable was used regardless of what it actually contains. This bleeds abstractions from fileio into the raw test by hardcoding into here what's set in fileio. We simply care here that it was set with (what we call) executable params, not the specifics of what those are. The test for what those are should live in fileio.
pkg/build/grub.go
Outdated
| if err != nil { | ||
| return "", fmt.Errorf("applying GRUB guestfish snippet: %w", err) | ||
| return "", fmt.Errorf("parsing guestfish template: %w", err) |
There was a problem hiding this comment.
Curious why you took the "GRUB" portion out. The intention was that it provided more specific information on where/what the error was. If we reuse the generic "parsing guestfish template" error elsewhere, that'll make debugging (admittedly slightly) less descriptive.
There was a problem hiding this comment.
I used that because the name of the template variable is guestfishSnippet. No issues putting GRUB back in though, will fix.
pkg/build/raw.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("writing modification script %s: %w", modifyScriptName, err) | ||
| return fmt.Errorf("parsing template: %w", err) |
There was a problem hiding this comment.
Similar to my other comment, I don't see the harm in including more information rather than less in an error message. The errors inside of template.Parse() don't give any details on what it was that failed, so including the name of the template being applied just makes our lives easier/quicker when it comes to debugging.
pkg/build/raw.go
Outdated
|
|
||
| filename := b.generateBuildDirFilename(modifyScriptName) | ||
| if err = os.WriteFile(filename, []byte(data), fileio.ExecutablePerms); err != nil { | ||
| return fmt.Errorf("writing modification script: %w", err) |
There was a problem hiding this comment.
Same comment here about the filename not being in the error. Even if it's in the wrapped error, I don't see the harm in including it here as well. I'm of the school of thought that more detail in error messages, even if it ends up being potentially redundant (which itself is contingent on os.WriteFile never removing that information), saves time while understanding an error message and debugging.
There was a problem hiding this comment.
I believe this is more of a style difference. I like keeping very simple wrappers and only including variables if / when absolutely necessary. No issues in bringing it back to what it used to be though, will fix.
pkg/build/scripts_test.go
Outdated
| @@ -56,7 +56,7 @@ func TestConfigureScripts(t *testing.T) { | |||
| fullEntryPath := filepath.Join(builder.context.CombustionDir, entry.Name()) | |||
| stats, err := os.Stat(fullEntryPath) | |||
| require.NoError(t, err) | |||
| assert.Equal(t, fs.FileMode(scriptMode), stats.Mode()) | |||
| assert.Equal(t, fs.FileMode(0o744), stats.Mode()) | |||
There was a problem hiding this comment.
Same here on the hardcoded permissions.
| "text/template" | ||
| ) | ||
|
|
||
| func Parse(name string, contents string, templateData any) (string, error) { |
There was a problem hiding this comment.
This is definitely going to make development easier. With this change, I can slap a debugger on my test and look at the applied template rather than having to crack open the file.
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
Signed-off-by: Atanas Dinov <[email protected]>
atanasdinov
force-pushed
the
file-perm
branch
from
9ac6983
to
7a13368
Compare
fileiofunctions in order to drop unnecessary and prone to missing additionalChmod()callsWriteFileimplementation to two separate partsWriteFilefor non-templated files andWriteTemplatefor templated onesWriteFileat that point was just a wrapper aroundos.WriteFileandWriteTemplatewas doing too many things for a single functiontemplatepackage was extracted and reused where applicable andWriteFilewas removed. This decouples templating from IO ops and further simplifies combustion components structure and testing