-
Notifications
You must be signed in to change notification settings - Fork 742
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
Add force modules restore support in CLI and VS Code #6382
Add force modules restore support in CLI and VS Code #6382
Conversation
2c7f853
to
822b1b7
Compare
It seems there is a problem with the test |
@majastrz Could you take a look at the code and tell me what you think. I will take a look at the (maybe?) race condition / unexpected status flag in |
Locks are advisory on Linux/OSX and mandatory on Windows. Not sure if that explains the difference in behavior - will take a look sometime this week. In reply to: 1086984349 |
4a0af82
to
63a86dc
Compare
It does, thanks for the pointer. I took a look at #if WINDOWS_BUILD
...
#else
// delete will succeed on Linux and Mac due to advisory nature of locks there
...
#endif |
4b90aa4
to
84f9bc5
Compare
Someone suggested to rename the command label, any thoughts on this? see #4758 |
{ | ||
var inputUri = PathHelper.FilePathToFileUrl(inputPath); | ||
var configuration = this.configurationManager.GetConfiguration(inputUri); | ||
var sourceFileGrouping = SourceFileGroupingBuilder.Build(this.fileResolver, this.moduleDispatcher, this.workspace, inputUri, configuration); | ||
var originalModulesToRestore = sourceFileGrouping.ModulesToRestore; | ||
|
||
// Ignore modules to restore logic, include all modules to be restored | ||
var allModules = sourceFileGrouping.SourceFilesByModuleDeclaration |
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.
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.
You're right, I wanted this to be more readable for the line after but it gets calculated all the time.
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.
My other worry with doing the calculations like this is that the logic could get out of sync with the logic called by Build()
.
Could we just pass a flag into Build()
so it ignores restore status and always acts like the module is not restored?
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.
Ok, so the idea would be to pass the forceModulesRestore
bool value all the way down to bicep/src/Bicep.Core/Workspaces/SourceFileGroupingBuilder.cs / SourceFileGrouping Build() / PopulateRecursive()
to centralize the logic for ModuleRestoreStatus
? in there we could force a value of ModuleRestoreStatus.Unknown
if we see that forceModulesRestore
is true
I thought about this. It would be cleaner if PopulateRecursive()
return all the modules if forceModulesRestore
. I was hesitant to go that route but you're probably right that it should be aware and handle part of that logic for the list of modules to be restored.
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.
Would we stop in SourceFileGroupingBuilder
or go even deeper into ModuleDispatcher.GetModuleRestoreStatus()
to completely override the status returned if asked for a forced restore? My bet would be down to GetModuleRestoreStatus()
.
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.
TBH, I originally thought about this while coding the feature, centralizing the modules to restore override logic, and told myself that it could be a future refactor. But you're right, why wait.
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.
@majastrz I changed how it works a bit and It should bring a better separation of concern. Let me know what you think of having a new ModuleDispatcher that is used only when a force restore is requested.
I will add test if you like this, just wanted to validate before writing tests more test for the new ModuleDispatcher
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.
Could we add a flag that makes the following true for all module refs?
Then, we wouldn't need any special handling for forced restore and could avoid the extra logging. The decision would be made by the same component as today and nothing would need to change in CLI. And we wouldn't need a separate dispatcher.
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.
It is a possibility to propagate a forceRestore value down to PopulateRecursive()
and short circuit the restoreStatus
to Unknown
when asked to force restore. It would be the quickest way to patch this.
IMO SourceFileGroupingBuilder and PopulateRecursive() are starting to get more responsibilities that they should. Maybe a decorator approach at the ModuleDispatcher level is too wide for our need. We could have the logic of ForceRestoreModuleDispatcher in a new interface to overrides only the RestoreModules & GetModuleRestoreStatus logic?
A bit like this:
// In CLI / CompilationService.cs
public async Task RestoreAsync(string inputPath, bool forceModulesRestore)
{
var inputUri = PathHelper.FilePathToFileUrl(inputPath);
var configuration = this.configurationManager.GetConfiguration(inputUri);
if(forceModulesRestore) {
this.moduleDispatcher.SetRestoreHandler(new ForceModuleRestoreHandler())
}
var sourceFileGrouping = SourceFileGroupingBuilder.Build(this.fileResolver, currentModuleDispatcher, this.workspace, inputUri, configuration);
var originalModulesToRestore = sourceFileGrouping.ModulesToRestore;
// RestoreModules() does a distinct but we'll do it also to prevent duplicates in processing and logging
var modulesToRestoreReferences = currentModuleDispatcher.GetValidModuleReferences(sourceFileGrouping.ModulesToRestore, configuration)
.Distinct()
.OrderBy(key => key.FullyQualifiedReference);
// restore is supposed to only restore the module references that are syntactically valid
await currentModuleDispatcher.RestoreModules(configuration, modulesToRestoreReferences);
...
Another idea would be to add a new ModuleRestoreStatus for the "cache deprecation" cleanup phase. Then we could properly handle the transition from one and another instead of hijacking the restore status ? It does implies a more work though.
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.
My other worry with doing the calculations like this is that the logic could get out of sync with the logic called by
Build()
.Could we just pass a flag into
Build()
so it ignores restore status and always acts like the module is not restored?
@majastrz I gave it a second look and though, thinking it would go faster like you mentioned since we were almost there.
If we do this, we really start to put logic in SourceFileGroupingBuilder
that take decisions away from ModuleDispatcher
.
- First it would override the
restoreStatus
- It needs also to override
restoreErrorBuilder
which would have returned anErrorDiagnostic
via anout
ModuleRequiresRestore
.
The code is written and it works, it really depends how we want the concerns to be separated.
I will commit the new code I have that support your suggestion and screenshot above. It revert the ForceRestoreModuleDispatcher
concept introduced previously.
I think we can deliver the force restore that way and create a new issue to rethink a bit the interaction and extensibility for components like ModuleDispatcher
& SourceFileGroupingBuilder
to prevent situations like this in the future.
IMO we should have been able to add this kind of functionality easily through aggregation/composition without touching much of the existing code, just extending/replacing behaviors.
1d84bd9
to
cc4b9c8
Compare
63a10a2
to
e712b24
Compare
…oupingBuilder to ease handle of force restore
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 forgot to say this earlier, but this is so cool. Thanks for building this! ❤️ |
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.
Thanks, guys, for your efforts. I am grateful that you are adding this behaviour |
Adding support for forced modules restore at command line (CLI) and VS Code
Azure CLI
bicep restore --force {$file}
VS Code
Force Modules Restore command, shortcut: CTRL+M R / CMD+M R
Fixes #4758