Skip to content
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.

Sign up for GitHub

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

nits and fixes #416

Merged
merged 1 commit into from
Nov 21, 2023
Merged

nits and fixes #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
### Why

11 changes: 7 additions & 4 deletions src/D2L.Bmx/BmxPaths.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
namespace D2L.Bmx;

internal static class BmxPaths {
public static readonly string USER_HOME_DIR = Environment.GetFolderPath( Environment.SpecialFolder.UserProfile );
public static readonly string BMX_DIR = Path.Join( USER_HOME_DIR, ".bmx" );
public static readonly string CACHE_DIR = Path.Join( BMX_DIR, "\\cache" );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point of Path.Join is that we don't need to worry about path separators.
including a backslash also results in incorrect behaviour on POSIX systems, where backslash can be part of a valid directory name.
@alex-fang0

public static readonly string SESSIONS_FILE_NAME = Path.Join( CACHE_DIR, "sessions" );
private static readonly string BMX_DIR = Path.Join(
Environment.GetFolderPath( Environment.SpecialFolder.UserProfile ),
Comment on lines +4 to +5
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USER_HOME_DIR and BMX_DIR are only used inside this static class to define other paths now, so they should be private or inlined.

".bmx" );

public static readonly string CACHE_DIR = Path.Join( BMX_DIR, "cache" );
public static readonly string CONFIG_FILE_NAME = Path.Join( BMX_DIR, "config" );
public static readonly string SESSIONS_FILE_NAME = Path.Join( CACHE_DIR, "sessions" );
public static readonly string SESSIONS_FILE_LEGACY_NAME = Path.Join( BMX_DIR, "sessions" );
public static readonly string UPDATE_CHECK_FILE_NAME = Path.Join( CACHE_DIR, "updateCheck" );
public static readonly string AWS_CREDS_CACHE_FILE_NAME = Path.Join( CACHE_DIR, "awsCreds" );
}
41 changes: 23 additions & 18 deletions src/D2L.Bmx/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,25 +210,30 @@
// start bmx
return await new CommandLineBuilder( rootCommand )
.UseDefaults()
.AddMiddleware( async (
context,
next
) => {
await UpdateChecker.CheckForUpdatesAsync( configProvider.GetConfiguration() );
//Creating new Cache directory and removing old files.
if( !Directory.Exists( BmxPaths.CACHE_DIR ) ) {
try {
Directory.CreateDirectory( BmxPaths.CACHE_DIR );
File.Delete( Path.Join( BmxPaths.BMX_DIR, "awsCredsCache" ) );
File.Delete( Path.Join( BmxPaths.BMX_DIR, "sessions" ) );
File.Delete( Path.Join( BmxPaths.BMX_DIR, "update_check" ) );
Comment on lines -222 to -224
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awsCredsCache and update_check are never released, so there's no need to handle them.

Moving (rather than deleting) sessions keeps user's old cached session, which is better UX.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what does 'released' mean here? I'm having trouble finding it on google because of the word 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean they're not in bmx v3.0.0, so "the people" never got them.

Copy link
Contributor

@gord5500 gord5500 Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see now. Thanks Chenfeng

} catch( Exception ex ) {
Console.Error.WriteLine( "Error cleaning up old files continuing..." );
.AddMiddleware(
middleware: async ( context, next ) => {
// initialize BMX directories
if( !Directory.Exists( BmxPaths.CACHE_DIR ) ) {
try {
// the cache directory is inside the config directory (~/.bmx), so this ensures both exist
Directory.CreateDirectory( BmxPaths.CACHE_DIR );
// move the old sessions file (v3.0-) into the new cache directory (v3.1+)
if( File.Exists( BmxPaths.SESSIONS_FILE_LEGACY_NAME ) ) {
File.Move( BmxPaths.SESSIONS_FILE_LEGACY_NAME, BmxPaths.SESSIONS_FILE_NAME );
}
} catch( Exception ex ) {
throw new BmxException( "Failed to initialize BMX directory (~/.bmx)", ex );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't swallow errors on directory init, because our file logic in the rest of the codebase now assumes the necessary directories always exist. If there's an error here and is ignored, we may encounter an unexpected error down the line, which results in non-descriptive error message for the user.

This does mean that we run the slight risk of erroring out on failing to move the sessions file, when it could be ignored. But that's a much smaller concern to me. If there's an error in moving that file, there's probably something odd about the user's filesystem and may be worth investigating anyway.

}
}
}
await next( context );
},
System.CommandLine.Invocation.MiddlewareOrder.ExceptionHandler + 1

await UpdateChecker.CheckForUpdatesAsync( configProvider.GetConfiguration() );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move the update checker below directory init, so we can assume existence of standard BMX directories inside of it.


await next( context );
},
// The default order for new middleware is after the middleware for `--help` & `--version` and can get short-circuited.
// We want our middleware (especially update checks) to almost always run, even on `--help` & `--version`,
// so we specify a custom order just after the exception handler (which is way before `--help` & `--version`).
order: MiddlewareOrder.ExceptionHandler + 1
Comment on lines +233 to +236
Copy link
Member Author

@cfbao cfbao Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't recall exactly why we specified a middleware order here, and had to check Slack history, so it's worth explaining in a comment.

)
.UseExceptionHandler( ( exception, context ) => {
Console.ResetColor();
Expand Down
5 changes: 1 addition & 4 deletions src/D2L.Bmx/UpdateChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
using System.Text.Json.Serialization;

namespace D2L.Bmx;
internal static class UpdateChecker {

internal static class UpdateChecker {
public static async Task CheckForUpdatesAsync( BmxConfig config ) {
try {
var cachedVersion = GetUpdateCheckCache();
Expand Down Expand Up @@ -68,9 +68,6 @@ private static async Task<string> GetLatestReleaseVersionAsync() {
}

private static void SaveLatestVersion( string version ) {
if( !Directory.Exists( BmxPaths.BMX_DIR ) ) {
Directory.CreateDirectory( BmxPaths.BMX_DIR );
}
if( string.IsNullOrWhiteSpace( version ) ) {
return;
}
Expand Down
Loading