-
Notifications
You must be signed in to change notification settings - Fork 91
Remind users to upgrade old (<21) Java and EOL Spring Boot/Framework versions #901
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
Remind users to upgrade old (<21) Java and EOL Spring Boot/Framework versions #901
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
src/views/workspaceNode.ts
Outdated
| return Jdtls.getProjects(this.nodeData.uri); | ||
| const result = await Jdtls.getProjects(this.nodeData.uri); | ||
| PromotionProvider.getInstance().checkJavaVersion(result); | ||
| return result; |
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 this method is called >1 times?
- The loadData function should only handle data retrieval. Performing a Java version validation at the same time as data fetching seems quite unreasonable. This is clearly a "side effect" and should be avoided.
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 this method is called >1 times?
Controls are implemented in a downstream function to prevent repeated notifications.
The loadData function should only handle data retrieval.
I'll seek to move the added logic out of loadData().
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.
I've updated the PR to check Java version on creation of ProjectNode (new ProjectNode(...)) - Java version is related to this type of node. Outside the scope the Java version data is hard to be read (it's saved in a protected property) without touching the property structure of classes.
67e3caa to
6adb53b
Compare
6cd7dd9 to
991c776
Compare
This comment was marked as outdated.
This comment was marked as outdated.
akaroml
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.
Please detect whether the App Mod ext is active. If it's inactive, no hint should be shown to the users.
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.
Please only make necessary changes to reduce the size of of the PR. In this file, the only change is the inserted line of:
"contributes.commands.java.view.triggerJavaUpgradeTool": "Upgrade dependencies",
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.
Formatting are restored in c2c4cf6.
src/constants.ts
Outdated
| export const DIAGNOSTICS_GROUP_ID_FOR_JAVA_ENGINE = "java-engine"; | ||
| export const DIAGNOSTICS_NAME_FOR_JAVA_ENGINE = "Java Engine"; | ||
| export const PROMOTION_DIAGNOSTIC_SOURCE = "Java"; | ||
| export const EARLIEST_JAVA_VERSION_NOT_TO_PROMPT = 21; |
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.
LATEST_JAVA_LTS_VERSION
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.
Done in 56084e6.
src/constants.ts
Outdated
|
|
||
| export namespace Upgrade { | ||
| export const DIAGNOSTICS_GROUP_ID_FOR_JAVA_ENGINE = "java-engine"; | ||
| export const DIAGNOSTICS_NAME_FOR_JAVA_ENGINE = "Java Engine"; |
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.
Use "java-upgrade-assistant" and "Java Upgrade Assistant" instead.
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.
Changed to java:* to align with the java-upgrade extension.
src/constants.ts
Outdated
| export namespace Upgrade { | ||
| export const DIAGNOSTICS_GROUP_ID_FOR_JAVA_ENGINE = "java-engine"; | ||
| export const DIAGNOSTICS_NAME_FOR_JAVA_ENGINE = "Java Engine"; | ||
| export const PROMOTION_DIAGNOSTIC_SOURCE = "Java"; |
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.
"Java Upgrade Assistant"
src/extension.ts
Outdated
| import * as path from "path"; | ||
| import { commands, Diagnostic, Extension, ExtensionContext, extensions, languages, | ||
| Range, tasks, TextDocument, TextEditor, Uri, window, workspace } from "vscode"; | ||
| import { |
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.
Please avoid unnecessary changes.
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.
Fixed in c3ee610.
src/syncHandler.ts
Outdated
| instrumentOperation(ENABLE_AUTO_REFRESH, () => this.enableAutoRefresh())(); | ||
| } else { | ||
| instrumentOperation(DISABLE_AUTO_REFRESH, () => {})(); | ||
| instrumentOperation(DISABLE_AUTO_REFRESH, () => { })(); |
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.
Avoid unnecessary changes.
| } else { | ||
| // in flat view | ||
| if (path.extname(uri.fsPath) === ".java" && node.uri && | ||
| Uri.parse(node.uri).fsPath === path.dirname(uri.fsPath)) { |
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.
Avoid unnecessary changes.
src/upgrade/upgradeManager.ts
Outdated
| ); | ||
| } | ||
|
|
||
| private async checkUpgradableComponents(folder: WorkspaceFolder) { |
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.
Since we are already in Project Explorer, we already know all the dependencies and the version of java runtimes. Can we work with the project metadata directly? wihtout parsing the POM/Gradle files manually? CC @jdneo
akaroml
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.
In general, I wasn't expecting so many code changes. The fat implementation of manually parsing everything is the root cause. As commented, please consider work with the project metadata directly. In the metadata, the runtime version and dependency versions should all be available.
akaroml
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.
In general, I wasn't expecting so many code changes. The fat implementation of manually parsing everything is the root cause. As commented, please consider work with the project metadata directly. In the metadata, the runtime version and dependency versions should all be available.
abfd8c7 to
fa488ef
Compare
|
@akaroml Thanks for the review!
The code is now working with |
... to java.dependency.enableDependencyCheckup` as `Diagnostics` is used as a VSCode-specific concept
wangmingliang-ms
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.
LGTM
wangmingliang-ms
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.
LGTM
src/upgrade/utility.ts
Outdated
| case UpgradeReason.DEPRECATED: | ||
| case UpgradeReason.END_OF_LIFE: | ||
| default: { | ||
| return `The current project is using ${packageDisplayName} ${currentVersion}, which has reached end of life. Do you want to upgrade to ${suggestedVersionName} (${suggestedVersionDescription})?`; |
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.
“which has reached end of life”
Can we be explicit about the time of "end of life"?
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.
Fixed by 4d271c1.
Consensus has been reached per offline discussion.
Components for Java engine/dependency upgrade reminder.