-
Notifications
You must be signed in to change notification settings - Fork 100
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.
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
Ensure only files are in file overview in config apply request #1395
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release-3.5.1 #1395 +/- ##
==============================================
Coverage 86.41% 86.41%
==============================================
Files 102 102
Lines 12585 12604 +19
==============================================
+ Hits 10875 10892 +17
Misses 1233 1233
- Partials 477 479 +2
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Proposed changes
Ensure only files are in file overview in config apply request.
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)