-
Notifications
You must be signed in to change notification settings - Fork 49
#1602: Being offline can block ide startup #1671
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
base: main
Are you sure you want to change the base?
#1602: Being offline can block ide startup #1671
Conversation
…beec/IDEasy into feature/1602-offline-startup-blocked
Pull Request Test Coverage Report for Build 21014707628Details
💛 - Coveralls |
hohwille
left a comment
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.
@maybeec thanks for your PR. This is a really great improvement that I like to merge.
I was not really aware of this problem that we check for offline and throw an exception before we check that the download may already been cached....
FYI: I always told the development team that I consider it as an anti-pattern to do an explicit online check and throw an offline exception prior to invoking network activity.
I created invokeNetworkTask as a reasonable alternative to get what we really want.
Example:
IDEasy/cli/src/main/java/com/devonfw/tools/ide/tool/mvn/MvnRepository.java
Lines 282 to 287 in 0c7f3d0
| return this.context.getNetworkStatus().invokeNetworkTask(() -> { | |
| URL xmlUrl = new URL(url); | |
| try (InputStream is = xmlUrl.openStream()) { | |
| return documentBuilder.parse(is); | |
| } | |
| }, url); |
This way we simply perform the network operation and if we get a network issue (e.g. IOException) then this is stored as the latest network error and considered for our offline/online status. Our artificial online check is just pinging github.com and it may be unlikely that this fails. However, I have already seen situation where that failed due to ZScaler interfering while some other tool download worked fine.
| * {@code resolvedVersion} (returning the existing installed version instead). | ||
| */ | ||
| protected void performToolInstallation(ToolInstallRequest request, Path installationPath) { | ||
| protected VersionIdentifier performToolInstallation(ToolInstallRequest request, Path installationPath, VersionIdentifier resolvedVersion) { |
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.
wouldn't it be easier to not change this signature and do the try-catch outside of performToolInstallation?
BTW: using exceptions for control flow is not nice but I everything else would require way more refactoring - so I guess this should be fine here.
|
Maybe we can include this great feature into 2026.01.001... |
| VersionIdentifier actualInstalledVersion = performToolInstallation(request, installationPath, resolvedVersion); | ||
| // If offline and could not download, actualInstalledVersion will be the old version, not resolvedVersion | ||
| // In that case, we need to recalculate the installation path for the actually installed version | ||
| if (!actualInstalledVersion.equals(resolvedVersion)) { | ||
| installationPath = getInstallationPath(edition, actualInstalledVersion); | ||
| } | ||
| return createToolInstallation(installationPath, actualInstalledVersion, true, processContext, additionalInstallation); |
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.
| VersionIdentifier actualInstalledVersion = performToolInstallation(request, installationPath, resolvedVersion); | |
| // If offline and could not download, actualInstalledVersion will be the old version, not resolvedVersion | |
| // In that case, we need to recalculate the installation path for the actually installed version | |
| if (!actualInstalledVersion.equals(resolvedVersion)) { | |
| installationPath = getInstallationPath(edition, actualInstalledVersion); | |
| } | |
| return createToolInstallation(installationPath, actualInstalledVersion, true, processContext, additionalInstallation); | |
| try { | |
| performToolInstallation(request, installationPath); | |
| } catch (CliOfflineException e) { | |
| // If we are offline and cannot download, check if we can continue with an existing installation | |
| ToolEditionAndVersion installed = request.getInstalled(); | |
| if ((installed != null) && (installed.getResolvedVersion() != null)) { | |
| this.context.warning("Cannot download {} in version {} because we are offline. Continuing with already installed version {}.", this.tool, resolvedVersion, installed.getResolvedVersion()); | |
| // If offline and could not download, actualInstalledVersion will be the old version, not resolvedVersion | |
| // In that case, we need to recalculate the installation path for the actually installed version | |
| installationPath = getInstallationPath(edition, installed.getResolvedVersion()); | |
| } | |
| // No existing installation available, re-throw the exception | |
| throw e; | |
| } | |
| return createToolInstallation(installationPath, actualInstalledVersion, true, processContext, additionalInstallation); |
Description
Fixes issue #1602: Being offline can block IDE startup when a tool update is available but not cached.
Changes
Core Fix
doDownload()to check if download is already cached before throwingCliOfflineExceptionin offline modeperformToolInstallation()to returnVersionIdentifierindicating which version was actually installedSupporting Changes
performToolInstallation()signatureTests
Added comprehensive test coverage in InstallCommandletTest.java:
testInstallCommandletOfflineWithCachedDownload: Fresh install with cached download succeedstestInstallCommandletOfflineWithoutCachedDownload: Fresh install without cache fails appropriatelytestInstallCommandletOfflineUpdateWithoutCachedDownload: Update scenario keeps old version when new version not cachedDocumentation
Testing
All offline scenario tests pass successfully:
Checklist
#1602: Being offline can block ide startupRelated Issue
Closes #1602