Adding update command #418
Conversation
github-actions
bot
added
language/csharp
size/L
|
PR #419 created to format .cs files. |
github-actions
bot
added
size/M
|
PR #420 created to format .cs files. |
|
PR #421 created to format .cs files. |
|
PR #422 created to format .cs files. |
|
PR #423 created to format .cs files. |
|
PR #424 created to format .cs files. |
|
PR #425 created to format .cs files. |
|
PR #427 created to format .cs files. |
|
PR #428 created to format .cs files. |
src/D2L.Bmx/BmxPaths.cs
Outdated
| @@ -11,4 +11,5 @@ internal static class BmxPaths { | |||
| 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" ); | |||
| public static readonly string OLD_BMX_VERSION_FILE_NAME = Path.Join( BMX_DIR, "bmx" ); | |||
There was a problem hiding this comment.
Tried running the update, and got this:
thoughts:
- would prefer that we hide the backup files in a subdirectory ("temp"? "old-exe"? or a better name). We made an effort to clean up the top level .bmx directory to only user facing things. Let's keep it clean. It should make cleanup safer too.
- The filename could be better formatted:
- Add a separator between "bmx" and the version string
- Use a different separator that's not dot to avoid confusion with the dots in the version string, e.g.
bmx-v1.2.3-old.bak - Consider adding a timestamp or a random string to the name. This may not be necessary, but might be nice if a user updated multiple BMX binaries of the same version on the machine.
- The variable
OLD_BMX_VERSION_FILE_NAMEis misnamed. It's not really used as a filename, but a prefix. However, if we put old binaries inside a subfolder, we only need to track the subfolder name, and not the filename prefix.
There was a problem hiding this comment.
Yea that makes sense! Added a temp folder for the old versions and millisecond timestamp for the file.
src/D2L.Bmx/GithubRelease.cs
Outdated
| } | ||
|
|
||
| internal static class GithubUtilities { | ||
| public static string GetReleaseVersion( GithubRelease? releaseData ) { |
There was a problem hiding this comment.
I'd prefer if we don't allow calling such a method on nulls at all. I'd also argue that we should return null (instead of empty string) on a non-null GithubRelease too if it doesn't have a version.
I explained the reasoning here: #259 (comment)
Actually... it looks like we always convert the string to a Version object after calling this method, so might as well return a Version here directly? Strongly typed API is always nice. (this suggestion is entirely optional)
Also, as Adipa said, moving this method into the GithubRelease record itself would be even better, and takes care of part of the null problem too.
The end result could look something like this for the caller:
var releaseData = await GetLatestReleaseDataAsync();
var version = releaseData?.GetVersion();
if( version is null ) {
// handle error - throw or early return
}
// happy pathCurrently, we're using empty string to avoid handling the error case, even though our data model totally permits a non-existent version. If GitHub does return a non-existent version somehow, BMX would throw an unexpected exception with non-descript error message.
Not falling back to empty string forces us to be consistent in our data model & behaviour, and handle all expected errors.
There was a problem hiding this comment.
Makes sense that we have consistency and clarity on returning nulls. Made some changes to put the methods into the record itself
| internal class UpdateHandler { | ||
|
|
||
| public async Task HandleAsync() { | ||
| using var httpClient = new HttpClient(); |
There was a problem hiding this comment.
We probably have enough GitHub interaction code to warrant a GitHubClient class. Then, ideally, I'd inject that into dependent classes via constructor parameters, and avoid instantiating new clients directly.
DI is generally nice (for testing and dependency/behaviour tracking), and consistent with our general coding practice.
I'm willing to let this PR through without DI for now, but I would like us to refactor this soon-ish.
|
PR #429 created to format .cs files. |
|
PR #430 created to format .cs files. |
src/D2L.Bmx/UpdateHandler.cs
Outdated
| } catch( Exception ex ) { | ||
| File.Move( backupPath, currentFilePath ); |
There was a problem hiding this comment.
Instead of moving the current file to backup path first then moving it back on error, how about downloading and extracting the new file into temp directory first, then do all the moves afterwards?
There was a problem hiding this comment.
I think for the TarFile extract we don't set or return the file name so I think it might be a bit complicated keeping track of which files to move in the directory if there are multiple updates running.
There was a problem hiding this comment.
Sorry, when I said "temp directory" I was actually thinking of a uniquely named subdirectory in the system temp directory, i.e. what's returned by Path.GetTempFileName.
Even if we use ~/.bmx/temp, we can also create more subdirectories in it with unique names, e.g.
~/.bmx/temp/58ba67a7-10f3-418c-a686-90894c2b6045/
~/.bmx/temp/b90477fc-469b-4651-8ace-0ab522c935c8/
...
we can generate and keep track of the UUID (or other types of unique identifiers e.g. timestamps?) in memory, and not worry about multiple runs interfering with each other.
There was a problem hiding this comment.
I made a commit trying out the temp directory first to push moving files to the end, seems a bit messy so I'm not sure if we should go for this. We can revert if needed
There was a problem hiding this comment.
The new change looks good. We can make it cleaner, but even as is it doesn't look too messy to me.
I do vastly prefer the new approach of "not moving files until we have everything ready".
| } else if( extension.Equals( ".gz", StringComparison.OrdinalIgnoreCase ) ) { | ||
| ExtractTarGzipFile( downloadPath, currentDirectory ); | ||
| } else { | ||
| throw new Exception( "Unknown archive type" ); |
There was a problem hiding this comment.
use BmxException with better error message
There was a problem hiding this comment.
This is already a nested exception (not sure if we can un-nest it without making the block messy) so I don't think the user will see it. I think the "Failed to update with new files" message should capture this tho
There was a problem hiding this comment.
ah I see. Didn't notice the nesting. I'd still like to avoid it, but it's good for now.
|
PR #431 created to format .cs files. |
|
PR #432 created to format .cs files. |
|
PR #433 created to format .cs files. |
Auto format liam_update_test Co-authored-by: DotNet Format Bot <[email protected]>
src/D2L.Bmx/UpdateHandler.cs
Outdated
| throw new Exception( "Unknown archive type" ); | ||
| } | ||
| } catch( Exception ex ) { | ||
| Directory.Delete( extractFolder, true ); |
There was a problem hiding this comment.
Boolean literals should almost always be named explicitly
| Directory.Delete( extractFolder, true ); | |
| Directory.Delete( extractFolder, recursive: true ); |
similarly for other Directory.Delete calls
src/D2L.Bmx/UpdateHandler.cs
Outdated
| string destinationPath = Path.GetFullPath( Path.Combine( decompressedFilePath!, entry.FullName ) ); | ||
| if( destinationPath.StartsWith( decompressedFilePath!, StringComparison.Ordinal ) ) { |
There was a problem hiding this comment.
decompressedFilePath isn't nullable
| string destinationPath = Path.GetFullPath( Path.Combine( decompressedFilePath!, entry.FullName ) ); | |
| if( destinationPath.StartsWith( decompressedFilePath!, StringComparison.Ordinal ) ) { | |
| string destinationPath = Path.GetFullPath( Path.Combine( decompressedFilePath, entry.FullName ) ); | |
| if( destinationPath.StartsWith( decompressedFilePath, StringComparison.Ordinal ) ) { |
Why
Need a command to update bmx with a new version if one is available