-
Notifications
You must be signed in to change notification settings - Fork 223
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
Adding version check to startup #2567
base: main
Are you sure you want to change the base?
Conversation
src/Service/Program.cs
Outdated
@@ -27,6 +27,13 @@ public class Program | |||
|
|||
public static void Main(string[] args) | |||
{ | |||
// Compare current version of DAB with latest (non-rc) version in NuGet. |
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 this change work for the docker scenario too?
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.
We'd need to test to be sure. My gut says no.
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.
If it fails to reach NuGet it does not emit a warning.
src/Service/Program.cs
Outdated
@@ -27,6 +28,13 @@ public class Program | |||
|
|||
public static void Main(string[] args) | |||
{ | |||
// Compare current version of DAB with latest (non-rc) version in NuGet. | |||
VersionChecker.GetVersions(out string? latestVersion, out string? currentVersion); | |||
if (!string.IsNullOrEmpty(latestVersion) && latestVersion != currentVersion) |
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.
what if people clone the repo, their currentVersion might be > latestVersion. Saying newer version is available doesn't make sense in that scenario, we should check for latestVersion > currentVersion
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.
Great idea, but left some questions and suggestions to consider.
latestVersion = FetchLatestNuGetVersion(); | ||
currentVersion = GetCurrentVersionFromAssembly(Assembly.GetCallingAssembly()); | ||
nugetVersion = FetchLatestNuGetVersion(); | ||
localVersion = ProductInfo.GetProductVersion(false); |
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: name the argument that is passed to GetProductVersion with value false
for better readability..
currentVersion = GetCurrentVersionFromAssembly(Assembly.GetCallingAssembly()); | ||
nugetVersion = FetchLatestNuGetVersion(); | ||
localVersion = ProductInfo.GetProductVersion(false); | ||
return string.IsNullOrEmpty(nugetVersion) || nugetVersion == localVersion; |
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.
What if the localVersion is > than the nugetVersion? Could happen when people clone our repo and we are in the process of developing 1.5, But nuget version will be 1.4 in that timeframe.
public class VersionCheckTests | ||
{ | ||
[TestMethod] | ||
public void GetVersions_LatestVersionNotNull() |
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.
public void GetVersions_LatestVersionNotNull() | |
public void GetVersions_NugetVersionNotNull() |
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.
Good to move to dab validate for starters. PR description needs to be updated accordingly.
Still waiting for checking what happens if currentVersion > nugetVersion - could happen when people clone the repo and use the cli from the cloned build.
Closes #3292
Why make this change?
It is easy to continue running an outdated version of Data API builder after a newer version has been released to NuGet. This PR updates the startup process, checking the current version against the latest version available on NuGet.
What is this change?
Azure.DataApiBuilder.Product.VersionChecker
.Program.Main()
.How was this tested?
Sample Request(s)