RepoRegistry: refactor format and access directly from verbs #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@derrickstolee FYI - opening this draft PR as WIP for feedback on the product changes, I still need to update the unit tests and functional tests. Thanks! |
Previously, the repo registry was owned completely by the service, and verbs that needed to register repos (e.g. 'scalar clone') would make a request to the service to perform the registration. As a consequence of this design, if the service were not running when repo was cloned, the repo would never be added to the registry, and the service would never run maintenance tasks for it. With the changes in this commit Scalar uses a new approach for the repo registry: - The registry is a directory populated with .repo files - 'scalar clone' will create a new .repo file for its repo (the name of the file being the SHA1 hash of its path) - The .repo file stores the repo's path and owner in JSON format - Scalar.Service will discover all the registered repos by scanning the registry directory each time a maintenance task runs As part of this change, the following code could be removed: - All registration related messages to/from the service and the scalar verbs - The concept of "active" registered repos. Now that there's not a mount process there's no need to track which repos are mounted (i.e. "active") - All methods in the registry interface that are no longer required Future work: - Add a new verb for registering and unregistering repos - Move the '--list-registered' option from the 'service' verb to the new verb mentioned above
wilbaker
force-pushed
the
refactor_reg_format
branch
from
d3c9523 to
8b332fb
Compare
mjcheetham
reviewed
Nov 5, 2019
For consistency with the underlying API, update GetRegisteredRepos to return an IEnumerable rather than a List. Additionally, update GetRegisteredRepos to use Directory.EnumerateFiles rather than DirectoryInfo.GetFileSystemInfos so that the code can lazily walk the .repo files.
For consistency with ScalarService.Mac.cs (and as a first step toward making the Windows service more unit testable) interact with the ScalarRepoRegistry via its interface rather than directly.
wilbaker
force-pushed
the
refactor_reg_format
branch
from
2fd51fb to
8915f90
Compare
Improve the naming used for repository root variables to make it clear that the registry expects the paths to already be normalized. Additionally, use the term 'userId' consistently, and remove the use of 'ownerSID'.
wilbaker
force-pushed
the
refactor_reg_format
branch
2 times, most recently
from
2ad74b1 to
5126ef3
Compare
wilbaker
commented
Nov 6, 2019
Scalar.UnitTests/Common/RepoRegistry/ScalarRepoRegistryTests.cs
Outdated
Show resolved
Hide resolved
mjcheetham
reviewed
Nov 6, 2019
Add an initial unit test to ScalarRepoRegistry that validates the registry creates its directory if it's missing from disk.
wilbaker
force-pushed
the
refactor_reg_format
branch
from
9d646e4 to
20336e6
Compare
Complete the unit tests for ScalarRepoRegistry. Additionally, the following improvements were made: - Renamed "TryRemoveRepo" to "TryUnregisterRepo" to better match the method for registering repos - Updated several Should extensions in the test to provide better validation
wilbaker
force-pushed
the
refactor_reg_format
branch
from
d66ad63 to
dd103b1
Compare
The one test still active in MacServiceTests was actually a test of ScalarVerbRunner.Mac. Remove MacServiceTests and replace it with ScalarVerbRunnerTests
wilbaker
changed the title
|
@derrickstolee @mjcheetham I've taken this PR out of draft/WIP. I still need to run some additional manual tests before merging, but otherwise the changes are ready for review. Please note the current build failures are due to the git installer being deleted from our MyGet feed. |
Add unit tests for MaintenanceTaskScheduler and its nested class MaintenanceTask. Additionally: - Update MockTracer to track calls to RelatedEvent so that they can be validated by the unit tests. - Update the code that generates CommonAssemblyVersion.cs so that the unit tests have access to internal members of Scalar.Service - Add GetTestRoot() to MockFileSystem that will return an appropriate mock root (based on the system's directory separator char)
Update the ServiceVerbTests for the new behavior of 'scalar service': - Only option is --list-registered - Supported on Mac - Tests cannot assume they are running in isolation, other repos on the machine may be registered
wilbaker
force-pushed
the
refactor_reg_format
branch
from
4106a61 to
da2f66b
Compare
Improve the logging of MaintenanceTaskScheduler: - Use StartActivity so that it's clear from the logs when Execute() has finished processing all repos - Log the name of the task enum value rather than its integer value
kewillford
reviewed
Nov 7, 2019
derrickstolee
approved these changes
Nov 7, 2019
Scalar.UnitTests/Common/RepoRegistry/ScalarRepoRegistryTests.cs
Outdated
Show resolved
Hide resolved
The only extension method, GetRegisteredReposForUser, was simply performing a Where(...) and it was only being used in one place in the code. That method (and IScalarRepoRegistryExtensions.cs) are now removed and the caller of GetRegisteredReposForUser performs the Where(...) itself.
This commit includes the following fixes/improvements - Make internals visible to Scalar.UnitTests.Windows - Make RunMaintenanceTaskForRepos to be private, and update unit tests to run against Execute rather than RunMaintenanceTaskForRepos - Renamed ShouldBeNonEmpty to ShouldNotBeNullOrEmpty - Renamed GetTestRoot() to GetMockRoot() - All unit tests in this patch series have been updated to use GetMockRoot() rather than "mock" - Simplified MockTracer's RelatedEvent implementations
- Log a summary of how repos were processed in RunMaintenanceTaskForRepos - Improve logging and readability in clone verb - Report failures to register in 'scalar clone' as errors
Use 'scalar' in the mock path rather than 'vfs4g'
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 16, 2020
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 16, 2020
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 16, 2020
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 16, 2020
This utility script was originally added to the VFSForGit project in an early bulk commit, as part of a trio of scripts to make it easier to mount, clone, and unmount a test repository in the ~/GVFSTest directory: microsoft/VFSForGit@785ccb4 As part of Scalar PR microsoft#216, though, the mount and unmount helper scripts were removed as no longer relevant. While the remaining script is still functional, its utility is limited as its restriction to a fixed ~/ScalarTest directory makes less sense without the matching mount/unmount scripts, so we remove it here.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 17, 2020
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 17, 2020
This utility script was originally added to the VFSForGit project in the early bulk commit microsoft/VFSForGit@785ccb4, as part of a trio of scripts to make it easier to mount, clone, and unmount a test repository in the ~/GVFSTest directory. As part of Scalar PR microsoft#216, though, the mount and unmount helper scripts were removed as no longer relevant. While the remaining script is still functional, its utility is limited as its restriction to a fixed ~/ScalarTest directory makes less sense without the matching mount/unmount scripts, so we remove it here.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 17, 2020
This utility script was originally added to the VFSForGit project in the early bulk commit microsoft/VFSForGit@785ccb4, as part of a trio of scripts to make it easier to mount, clone, and unmount a test repository in the ~/GVFSTest directory. As part of Scalar PR microsoft#216, though, the mount and unmount helper scripts were removed as no longer relevant. While the remaining script is still functional, its utility is limited as its restriction to a fixed ~/ScalarTest directory makes less sense without the matching mount/unmount scripts, so we remove it here.
chrisd8088
added a commit
to chrisd8088/scalar
that referenced
this pull request
Aug 17, 2020
In PR microsoft#216 extensive work was done to remove some of the repository registry support of the Scalar service, and many tests no longer required the MacTODO.NeedsServiceVerb category flag and could be successfully executed on the Mac. One test which appears to have been missed was the SecondCloneSucceedsWithMissingTrees() test in SharedCacheTests.cs; however, it also succeeds on the Mac now and so we can remove this functional test category entirely.
This was referenced Aug 17, 2020
derrickstolee
added a commit
that referenced
this pull request
Aug 17, 2020
This utility script was originally added to the VFSForGit project in the early bulk commit microsoft/VFSForGit@785ccb4, as part of a trio of scripts to make it easier to mount, clone, and unmount a test repository in the `~/GVFSTest` directory. In PR #216, though, the mount and unmount helper scripts were removed as no longer relevant to Scalar. While the remaining script is still functional, its utility is limited as its restriction to a fixed `~/ScalarTest` directory makes less sense without the matching mount/unmount scripts, so we remove it here.
derrickstolee
added a commit
that referenced
this pull request
Aug 17, 2020
…and Windows-only option Prior to starting work to support runs of the functional test suite on Linux, we can simplify some of the tests marked `MacTODO` as no longer needing that designation and also simplify some of the Windows-related configuration and options. On the Windows side of thing, we can remove the second invocation of `excludeCategories.Add(Categories.MacOnly)` because it is always performed on Windows in the `if/else` block just above it. We also only parse the `--windows-only` functional test command-line option when running on Windows, as it has no effect on macOS since the `WindowsOnly` tests are always excluded there. On the macOS side of things, there is one test still marked `MacTODO.NeedsServiceVerb` which appears to have been missed in PR #216, where that category was removed from many other tests. This `SecondCloneSucceedsWithMissingTrees` test succeeds on macOS as written, so it seems OK to remove the `MacTODO` flag on it. The other macOS change we make is to remove the `MacTODO.NeedsScalarConfig` category, which currently applies to all the tests in `MultiEnlistmentTests/ConfigVerbTests.cs`. These tests are also marked with `NeedsUpdatesForNonVirtualizedMode` and therefore will continue to _not_ run even after this change. (However, limited testing by disabling the `config` verb's "elevated privileges" [checks](https://github.com/microsoft/scalar/blob/ef5c1bc753d0e129a2ca27c0e4daee78a27d1d30/Scalar/CommandLine/ConfigVerb.cs#L95) and removing `NeedsUpdatesForNonVirtualizedMode` from the `ConfigVerbTests` indicates that, in fact, these tests would all succeed on macOS/Linux with some further adjustments. But for now we just remove the "extra" `MacTODO` exclusion on these tests.) These two changes allow us to remove two of the three `MacTODO` categories entirely. We also do minor housekeeping by fixing one typo in the Watchman `shutdown-server` command.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #111
TODOs for this PR
Previously, the repo registry was owned completely by the
service, and verbs that needed to register repos (e.g.
'scalar clone') would make a request to the service to
perform the registration.
As a consequence of this design, if the service were not
running when repo was cloned, the repo would never be added
to the registry, and the service would never run maintenance
tasks for it.
With the changes in this commit Scalar uses a new approach
for the repo registry:
name of the file being the SHA1 hash of its path)
by scanning the registry directory each time a maintenance
task runs
As part of this change, the following code could be removed:
the scalar verbs
not a mount process there's no need to track which repos
are mounted (i.e. "active")
required
Future work:
verb to the new verb mentioned above