-
Notifications
You must be signed in to change notification settings - Fork 751
feat(amazonq): java21 support #6414
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
Conversation
|
TO-DO: add changelog |
| message.tabID | ||
| ) | ||
|
|
||
| if (fromJDKVersion === JDKVersion.JDK21 && toJDKVersion === JDKVersion.JDK17) { |
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 block this but allow other downgrades? e.g. 21 -> 11
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.
Only this one is possible
The source Java versions we show in dropdown form are: 8, 11, 17, 21
The target Java versions we show in dropdown form are: 17, 21
So the only downgrade possible for user to select is 21 -> 17
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.
So in other words you don't allow downgrades. This is a confusing if statement if that is the original intent, can we maybe check that source version is not >= target version? Or a comment at the very least.
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.
Added 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.
Added another comment to make it more specific but after that LGTM
|
|
||
| export const AWSTemplateCaseInsensitiveKeyWords = ['cloudformation', 'cfn', 'template', 'description'] | ||
|
|
||
| // TO-DO: make sure all Strings look good for Java 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.
Are you still working on this TODO or did we just want to keep it in?
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.
Strings should be good, will remove this comment when confirmed
| 'This diff patch covers the set of upgrades for Springboot, JUnit, and PowerMockito frameworks.', | ||
| 'This diff patch covers the set of upgrades for Springboot, JUnit, and PowerMockito frameworks in Java 17.', | ||
| 'Prepare minimal upgrade to Java 21': | ||
| 'This diff patch covers the set of upgrades for Springboot, JUnit, and PowerMockito frameworks in Java 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.
I haven't looked at the closures for java 21 (I can look), but assuming they make the same corresponding upgrades as the first patch for java 17
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.
Yes will confirm, but that should be true
| const sourceLanguageVersion = `JAVA_${transformByQState.getSourceJDKVersion()}` | ||
| const targetLanguageVersion = `JAVA_${transformByQState.getTargetJDKVersion()}` | ||
| // TO-DO: remove these logs statements | ||
| console.log('sourceLanguageVersion', sourceLanguageVersion) |
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.
Assuming you'll be removing these
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.
Oops I just saw the GitHub comments my bad!
|
Need to receive confirmation that backend is ready for Java 21, so converting this PR to draft so that it is not accidentally merged, but it is ready. |
Co-authored-by: Maxim Hayes <[email protected]>
|
Ready to merge for 2/13 release |
|
/runIntegrationTests |
## Problem Support transformations to Java 21. ## Solution Add support. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: David Hasani <[email protected]> Co-authored-by: Maxim Hayes <[email protected]>
Problem
Support transformations to Java 21.
Solution
Add support.
feature/xbranches will not be squash-merged at release time.