Add force modules restore support in CLI and VS Code

[slapointe]

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

• I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
• I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
• I have appropriate test coverage of my new feature

[slapointe]

It seems there is a problem with the test ForceModuleRestoreWithStuckFileLockShouldFailAfterTimeout on Linux and OSX. It runs on Windows, but seems to have problems on other platform. The test use a lock on file to simulate, pretty much same test logic as in ModuleRestoreWithStuckFileLockShouldFailAfterTimeout. Is there a subtility on these platform regarding file locks?

[slapointe]

@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 ModuleRestoreWithStuckFileLockShouldFailAfterTimeout test when not on Windows.

[majastrz]

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

[slapointe]

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

It does, thanks for the pointer. I took a look at FileLockTests.cs and the problematic test now has the same logic.

#if WINDOWS_BUILD
                ...
#else
                // delete will succeed on Linux and Mac due to advisory nature of locks there
                ...
#endif

[slapointe]

Someone suggested to rename the command label, any thoughts on this? see #4758

[majastrz]

allModules

allModules gets calculated even if the user is not forcing, but it's only used with force.

[slapointe]

You're right, I wanted this to be more readable for the line after but it gets calculated all the time.

[majastrz]

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?

[slapointe]

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.

[slapointe]

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().

[slapointe]

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.

[slapointe]

@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

[majastrz]

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.

[slapointe]

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.

[slapointe]

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.

1. First it would override the restoreStatus
2. It needs also to override restoreErrorBuilder which would have returned an ErrorDiagnostic via an out 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.

[majastrz]

[majastrz]

I forgot to say this earlier, but this is so cool. Thanks for building this! ❤️

[slapointe]

Thanks again @majastrz and @shenglol for your help and reflexion on this one! We just need to merge this and it's ON for next release. =)

[shenglol]

[kathodion]

Thanks, guys, for your efforts. I am grateful that you are adding this behaviour