Skip to content

Ensure only files are in file overview in config apply request #1395

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.

Sign up for GitHub

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

Merged
dhurley merged 4 commits into from
Nov 19, 2025
Merged

Ensure only files are in file overview in config apply request #1395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc60627
Ensure only files are in file overview in config apply request
dhurley Nov 18, 2025
76d9dbd
Add more unit tests and logging
dhurley Nov 18, 2025
bec50ce
Clean up
dhurley Nov 19, 2025
bea3f91
Update log message
dhurley Nov 19, 2025
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
45 changes: 35 additions & 10 deletions internal/file/file_manager_service.go
View file
Open in desktop
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,26 +375,49 @@ 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 •
edited
Loading

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

@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.

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

Copy link
Collaborator Author

@dhurley dhurley Nov 19, 2025

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) {
modifiedFile.Action = model.Add

// 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() {
modifiedFile.Action = model.Update
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() {
modifiedFile.Action = model.Update
}

// If file doesn't exist on disk.
// Treat it as adding a new file.
if fileStats, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) {
modifiedFile.Action = model.Add
fileDiff[fileName] = modifiedFile

continue
// 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.
} else if statErr == nil {
if fileStats.IsDir() {
return nil, fmt.Errorf(
"unable to create file %s since a directory with the same name already exists on the data plane",
fileName,
)
}

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() {
modifiedFile.Action = model.Update
fileDiff[fileName] = modifiedFile
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions internal/file/file_manager_service_test.go
View file
Open in desktop
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 Down
Loading