-
Notifications
You must be signed in to change notification settings - Fork 89
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
base: main
Are you sure you want to change the base?
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.
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
@@ -35,6 +35,13 @@ export namespace ExtensionName { | |||
export const JAVA_LANGUAGE_SUPPORT: string = "redhat.java"; | |||
} | |||
|
|||
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
@@ -2,8 +2,10 @@ | |||
// Licensed under the MIT license. | |||
|
|||
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
@@ -26,7 +27,7 @@ class SyncHandler implements Disposable { | |||
if (autoRefresh) { | |||
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.
src/syncHandler.ts
Outdated
@@ -122,7 +124,7 @@ class SyncHandler implements Disposable { | |||
} 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
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.
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 |
src/constants.ts
Outdated
export namespace Upgrade { | ||
export const DIAGNOSTICS_GROUP_ID_FOR_JAVA_RUNTIME = "java-upgrade-assistant"; | ||
export const DIAGNOSTICS_NAME_FOR_JAVA_RUNTIME = "Java Upgrade Assistant"; | ||
export const LATEST_JAVA_LTS_VESRION = 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.
why is only Java here, where is SpringBoot...? is this used outside metadataManager?
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 related metadata are now moved into dependency.data.ts
.
package.json
Outdated
@@ -288,6 +288,16 @@ | |||
"command": "java.view.package.renameFile", | |||
"title": "%contributes.commands.java.view.package.renameFile%", | |||
"category": "Java" | |||
}, | |||
{ | |||
"command": "java.view.triggerJavaUpgradeTool", |
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.
how about "xxx.upgradeRuntimeAndFrameworks" or "xxx.upgradeWithCopilot"
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.
src/settings.ts
Outdated
@@ -108,6 +108,10 @@ export class Settings { | |||
return workspace.getConfiguration("java.dependency").get("refreshDelay", 2000); | |||
} | |||
|
|||
public static getShowUpgradeReminder() { |
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.
isDependencyDiagnosingEnabled()
?
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 Settings.getEnableDependencyDiagnostics()
- cabb469
IFile existingPom = proj.getFile("pom.xml"); | ||
if (existingPom.exists()) { | ||
projectNode.setMetaDataValue(POM_PATH, existingPom.getLocation().toOSString()); | ||
} |
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.
is this still needed? we don't need to show diagnostics according to the adjusted scope.
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.
Nope for now. I'll remove parts related to POM path parsing.
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.
Removed in 2789537
private isCandidate() { | ||
return this.context?.globalState.get<boolean>(IS_CANDIDATE_KEY, true); | ||
} | ||
|
||
private setCandidate(isCandidate: boolean) { | ||
this.context?.globalState.update(IS_CANDIDATE_KEY, isCandidate); | ||
} |
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.
not used?
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 realized it should actually be changed to read from/write to the setting item java.dependency.enableDependencyDiagnosing
. Will change later.
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.
The button should change to workspace-level setting rather than user-level setting.
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 1f0a36f
src/constants.ts
Outdated
export const DIAGNOSTICS_GROUP_ID_FOR_JAVA_RUNTIME = "java-upgrade-assistant"; | ||
export const DIAGNOSTICS_NAME_FOR_JAVA_RUNTIME = "Java Upgrade Assistant"; | ||
export const LATEST_JAVA_LTS_VESRION = 21; | ||
export const SESSION_COUNT_BEFORE_NOTIFICATION_RESHOW = 3; |
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.
is this also used outside the notificationManager?
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.
Moved most strings in their own files - 56084e6.
src/constants.ts
Outdated
} | ||
|
||
export namespace Upgrade { | ||
export const PACKAGE_ID_FOR_JAVA_RUNTIME = "java-runtime:*"; |
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:*
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 76a8261.
src/upgrade/upgradeManager.ts
Outdated
|
||
const DEFAULT_UPGRADE_PROMPT = "Upgrade Java project dependency to latest version."; | ||
|
||
function getJavaIssues(data: INodeData): UpgradeIssue[] { |
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.
telemeter detected upgrade issues.
dependency name
current 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.
notificationMessage, | ||
BUTTON_TEXT_UPGRADE, | ||
BUTTON_TEXT_NOT_NOW, | ||
BUTTON_TEXT_DONT_SHOW_AGAIN); |
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.
telemeter user actions.
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 e25d0b6.
….dependency.enableDependencyDiagnostics`
…_NOTIFICATION_RESHOW` to individual files
Components for Java engine/dependency upgrade reminder.