-
Notifications
You must be signed in to change notification settings - Fork 311
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
Implementing Repository Management actions + Database changes. #3836
base: main
Are you sure you want to change the base?
Conversation
* Adding the Repository tool * WIP * View is set up. Test ata as well. * Aligning columns. Adding column spacing. * Reverting this change * Update tools/RepositoryManagement/DevHome.RepositoryManagement/Strings/en-us/Resources.resw Co-authored-by: Kristen Schau <[email protected]> * Adressing comments * Revert "Reverting this change" This reverts commit 711dd78. * Moving to experimental * Disabling by default * Removing from experimental. Addressing comments. * Reverting to main * Update tools/RepositoryManagement/DevHome.RepositoryManagement/DevHome.RepositoryManagement.csproj Co-authored-by: Kristen Schau <[email protected]> * Update tools/RepositoryManagement/DevHome.RepositoryManagement/Extensions/ServiceExtensions.cs Co-authored-by: Kristen Schau <[email protected]> * Removing duplicate xaml code * Adding to the mermaid diagram * Got lost in the shuffle * Removing a refrence --------- Co-authored-by: Kristen Schau <[email protected]>
* WIP * EF works. Can save data. * Can read and write. :) * Repos are added when cloned. * Putting save/load into a different class * Adding default values * Cleaning up names and using statements * More comments * Removing refrence to the public nuget * Restoring the nuget config * Adding a test. Better defining dates * Adding some tests * More comments. * Better path comparison. * Removing unused equals code * Adding more comments. Making new migrations
…thub.com/microsoft/devhome into user/dhoehna/RepositoryManagementFeature
common/TelemetryEvents/DevHomeDatabase/DevHomeDatabaseContextEvent.cs
Outdated
Show resolved
Hide resolved
common/TelemetryEvents/RepositoryManagement/RepositoryLineItemErrorEvent.cs
Show resolved
Hide resolved
database/DevHome.Database/DatabaseModels/RepositoryManagement/Repository.cs
Outdated
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Outdated
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Show resolved
Hide resolved
...itoryManagement/DevHome.RepositoryManagement/ViewModels/RepositoryManagementItemViewModel.cs
Outdated
Show resolved
Hide resolved
@@ -49,4 +49,7 @@ public static class KnownPageKeys | |||
public static readonly string Feedback = "DevHome.Settings.ViewModels.FeedbackViewModel"; | |||
public static readonly string Environments = "DevHome.Environments.ViewModels.LandingPageViewModel"; | |||
public static readonly string SetupFlow = "DevHome.SetupFlow.ViewModels.SetupFlowViewModel"; | |||
|
|||
// Will not work with navigation service natively. Used for the dictionary in SetupFlowViewModel | |||
public static readonly string RepositoryConfiguration = "RepositoryConfiguration"; |
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 the full path not work here? I'm not able to figure out why this breaks the pattern above. If it's only used in a specific place, maybe it doesn't need to be in the KnownPageKeys list? Not sure
namespace DevHome.Common.TelemetryEvents.DevHomeDatabase; | ||
|
||
[EventData] | ||
public class DevHomeDatabaseEvent : EventBase |
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.
Nit: Lets not prepend DevHome to any names.
public class DevHomeDatabaseEvent : EventBase | |
public class DatabaseEvent : EventBase |
Is DatabaseEvent verbose enough to pinpoint the event? Maybe we can rename it. Same comments about DevHomeDatabaseContextEvent .
public class DevHomeDatabaseContext : DbContext | ||
{ | ||
public DbSet<Repository> Repositories { get; set; } | ||
private const string DatabaseFileName = "DevHome.db"; |
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.
Should probably add a Const value for this file name/path and add it somewhere central (like config?).
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.
Nit: I'm also not a fan of the name "DevHome.db". Mabye something like "DevHomeMachineData.db" to reflect that this is data about what's on the machine (at least that's what I think this is for) :)
repositoryEntity.Property(x => x.RepositoryClonePath).HasDefaultValue(string.Empty).IsRequired(true); | ||
repositoryEntity.Property(x => x.RepositoryName).HasDefaultValue(string.Empty).IsRequired(true); | ||
repositoryEntity.Property(x => x.CreatedUTCDate).HasDefaultValueSql("datetime()"); | ||
repositoryEntity.Property(x => x.UpdatedUTCDate).HasDefaultValue(new DateTime(1, 1, 1, 0, 0, 0, DateTimeKind.Utc)); |
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.
Is this to indicate when the Repo was updated locally? If so, it might need to be the same as CreatedUTCDate. I guess it depends on how it's used.
Action = action; | ||
} | ||
|
||
public DevHomeDatabaseContextEvent(string action, Exception ex) |
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.
Nit: I just realized that this is a failure event. Maybe the name needs to be changed to reflect that this is an error.
var existingRepository = GetRepository(repositoryName, cloneLocation); | ||
if (existingRepository != null) | ||
{ | ||
_log.Information($"A Repository with name {repositoryName} and clone location {cloneLocation} exists in the repository already."); |
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.
Nit: Should be error/warning if this causes MakeRepository() to fail.
@@ -43,6 +44,10 @@ public SetupFlowViewModel( | |||
SetupFlowOrchestrator orchestrator, | |||
PackageProvider packageProvider) | |||
{ | |||
_navigationTargets.Add(_creationFlowNavigationParameter, StartCreationFlow); | |||
_navigationTargets.Add("StartQuickstartPlayground", StartQuickStartFlow); |
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.
Nit: Can add _quickstartNavigationParameter back in and replace this
private void SendTelemetryAndLogError(string operation, Exception ex) | ||
{ | ||
TelemetryFactory.Get<ITelemetry>().LogError( | ||
ErrorEventName, |
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.
Better to just add the "DevHome_RepositoryLineItemError_Event" string here and delete the class var.
{ | ||
_log.Error(ex, $"Cound not move repository to the selected location."); | ||
TelemetryFactory.Get<ITelemetry>().Log( | ||
EventName, |
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.
Same comment as the other event. No need for this variable.
|
||
public class RepositoryManagementDataAccessService | ||
{ | ||
private const string EventName = "DevHome_RepositoryData_Event"; |
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.
IMO we should just use the string directly in the Telemetry call, since that's clearer.
} | ||
catch (Exception ex) | ||
{ | ||
_log.Error(ex, "Exception when updating the clone location."); |
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.
Nit:
_log.Error(ex, "Exception when updating the clone location."); | |
_log.Error(ex, "Exception when removing the repository."); |
} | ||
catch (Exception ex) | ||
{ | ||
_log.Error(ex, "Exception when updating the clone location."); |
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.
Nit:
_log.Error(ex, "Exception when updating the clone location."); | |
_log.Error(ex, "Exception when setting repository hidden status."); |
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.
LGTM, nothing blocking, and TODO's are being taken care of in https://github.com/microsoft/devhome/tree/user/dhoehna/DatabaseMigrationsAndTodos
Summary of the pull request
This PR allows users to
All from the repository management page! Yay!
In addition to logic changes. I made some database changes
IsHidden
is part of the Repository entityRepositoryManagementDataAccessService
toDevHome.Database
to prevent a circular dependency. Now I can callMakeConfigurationFileFromRepoAndGit
without moving the code base to a new project.Repository
test.Repository
Another change is in Navigation. Changed navigation to use a Dictionary instead of multiple if/else.
References and relevant issues
Detailed description of the pull request / Additional comments
This PR has many TODO:'s and they will be removed in another PR this week as I address them.
This feature is still experimental.
Validation steps performed
PR checklist