Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions internal/file/file_manager_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context,

fms.fileActions = diffFiles

slog.DebugContext(ctx, "Executing config apply file actions", "actions", diffFiles)

rollbackTempFilesErr := fms.backupFiles(ctx)
if rollbackTempFilesErr != nil {
return model.Error, rollbackTempFilesErr
Expand Down Expand Up @@ -373,24 +375,58 @@ func (fms *FileManagerService) DetermineFileActions(
for _, modifiedFile := range modifiedFiles {
fileName := modifiedFile.File.GetFileMeta().GetName()
currentFile, ok := filesMap[fileName]
// default to unchanged action
modifiedFile.Action = model.Unchanged

// if file is unmanaged, action is set to unchanged so file is skipped when performing actions
// If file is unmanaged, action is set to unchanged so file is skipped when performing actions.
Copy link

@vrmare vrmare Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function can be refactored for simplicity? I imagine existing manifest (source of truth) is maintained as a virtual FS in memory. Then single loop over modified files can be applied to the VFS.

Something like this:

type VirtualFS struct {
    files map[string]SomeFileCache
    mu    sync.RWMutex
}

func (vfs *VirtualFS) ApplyChanges(modifiedFiles map[string]SomeFileCache) ([]FileAction, error) {}

The idea is that VFS holds canonical state.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bigger change I believe and happy to discuss outside this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can take a look at this and do any changes in a separate PR

if modifiedFile.File.GetUnmanaged() {
slog.DebugContext(ctx, "Skipping unmanaged file updates", "file_name", fileName)
continue
}
// if file doesn't exist in the current files, file has been added
// set file action
if _, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) {

// If file currently exists on disk, is being tracked in manifest and file hash is different.
// Treat it as a file update.
if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() {
slog.DebugContext(ctx, "Tracked file requires updating", "file_name", fileName)
modifiedFile.Action = model.Update
fileDiff[fileName] = modifiedFile

continue
}

fileStats, statErr := os.Stat(fileName)

// If file doesn't exist on disk.
// Treat it as adding a new file.
if errors.Is(statErr, os.ErrNotExist) {
slog.DebugContext(ctx, "New untracked file needs to be created", "file_name", fileName)
modifiedFile.Action = model.Add
fileDiff[fileName] = modifiedFile

continue
// if file currently exists and file hash is different, file has been updated
// copy contents, set file action
} else if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() {
}

// If there is an error other than not existing, return that error.
if statErr != nil {
return nil, fmt.Errorf("unable to stat file %s: %w", fileName, statErr)
}

// If there is a directory with the same name, return an error.
if fileStats.IsDir() {
return nil, fmt.Errorf(
"unable to create file %s since a directory with the same name already exists",
fileName,
)
}

// If file already exists on disk but is not being tracked in manifest and the file hash is different.
// Treat it as a file update.
metadataOfFileOnDisk, err := files.FileMeta(fileName)
if err != nil {
return nil, fmt.Errorf("unable to get file metadata for %s: %w", fileName, err)
}

if metadataOfFileOnDisk.GetHash() != modifiedFile.File.GetFileMeta().GetHash() {
slog.DebugContext(ctx, "Untracked file requires updating", "file_name", fileName)
modifiedFile.Action = model.Update
fileDiff[fileName] = modifiedFile
}
Expand Down
34 changes: 29 additions & 5 deletions internal/file/file_manager_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -763,12 +763,12 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
modifiedFiles: map[string]*model.FileCache{
addTestFile.Name(): {
File: &mpi.File{
FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(fileContent)),
FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(addFileContent)),
},
},
updateTestFile.Name(): {
File: &mpi.File{
FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(fileContent)),
FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(updatedFileContent)),
},
},
deleteTestFile.Name(): {
Expand All @@ -782,10 +782,10 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
FileMeta: protos.FileMeta(deleteTestFile.Name(), files.GenerateHash(fileContent)),
},
updateTestFile.Name(): {
FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(fileContent)),
FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(updatedFileContent)),
},
addTestFile.Name(): {
FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(fileContent)),
FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(addFileContent)),
},
},
expectedCache: make(map[string]*model.FileCache),
Expand All @@ -805,6 +805,24 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
expectedContent: make(map[string][]byte),
expectedError: nil,
},
{
name: "Test 4: File is actually a directory",
allowedDirs: []string{tempDir},
modifiedFiles: map[string]*model.FileCache{
tempDir: {
File: &mpi.File{
FileMeta: protos.FileMeta(tempDir, files.GenerateHash(fileContent)),
},
},
},
currentFiles: make(map[string]*mpi.File),
expectedCache: map[string]*model.FileCache(nil),
expectedContent: make(map[string][]byte),
expectedError: fmt.Errorf(
"unable to create file %s since a directory with the same name already exists",
tempDir,
),
},
}

for _, test := range tests {
Expand All @@ -828,7 +846,13 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) {
test.currentFiles,
test.modifiedFiles,
)
require.NoError(tt, fileActionErr)

if test.expectedError != nil {
require.EqualError(tt, fileActionErr, test.expectedError.Error())
} else {
require.NoError(tt, fileActionErr)
}

assert.Equal(tt, test.expectedCache, diff)
})
}
Expand Down
Loading