Skip to content
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

Update JDK to 17. #177

Merged
merged 4 commits into from Mar 2, 2023
Merged

Update JDK to 17. #177

merged 4 commits into from Mar 2, 2023

Conversation

armogur
Copy link
Contributor

@armogur armogur commented Jan 24, 2023

Issue #, if available: 155

Description of changes:
Switch from javax.jms.* to jakarta.jms.* namespace.
Update testing libraries and maven plugins.
Update JDK to 17.
Code cleanup, fixed typos in comments, used newer assertions in test code.

Switch from javax.jms.* to jakarta.jms.* namespace.
Update testing libraries and maven plugins.
@armogur armogur marked this pull request as draft January 24, 2023 07:21
@armogur armogur marked this pull request as ready for review January 24, 2023 07:21
@AlexRosier
Copy link

AlexRosier commented Feb 8, 2023

Can somebody from AWS / with write access please look at this. This would enable us to switch to spring boot 3
@ziyanli-amazon

@armogur
Copy link
Contributor Author

armogur commented Feb 8, 2023

Can somebody from AWS / with write access please look at this. This would enable us to switch to spring boot 3

@AlexRosier at this moment no one will receive your message as there is no other participant, maybe you should mention someone from AWS java team.

@volkanto
Copy link
Contributor

volkanto commented Feb 9, 2023

@ziyanli-amazon fyi ☝️

@@ -179,26 +179,26 @@ public QueueSession createQueueSession(boolean transacted, int acknowledgeMode)
* transaction and acknowledge mode.
*/
@Override
public Session createSession(boolean transacted, int acknowledgeMode) throws JMSException {
public Session createSession(boolean transacted, int sessionMode) throws JMSException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to change the variable naming here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh, right, something went sideways from my side, just checked the signature of the method, I will revert it back tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@armogur thank you! I left some minor comments there, the majorities are format related. Please help us retain a good and uniformed package code style and convention. thanks a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, please check.


import com.amazon.sqs.javamessaging.message.SQSObjectMessage;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do explicit import like
import static org.junit.jupiter.api.Assertions.assertNotNull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in every file.

import static com.amazon.sqs.javamessaging.SQSMessagingClientConstants.APPROXIMATE_RECEIVE_COUNT;
import static com.amazon.sqs.javamessaging.SQSMessagingClientConstants.JMSX_DELIVERY_COUNT;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do explicit import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in every file, please check.

Replaced JUnit's assertEquals with assertJ's assertThat.
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.4.1</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, did you see a lot of [WARNING] after upgrading the javadoc version here without having the <doclint>none</doclint> field?

Copy link
Contributor

@ziyanli-amazon ziyanli-amazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this @armogur. I have also published a PR to java v1, awaiting for internal review.

@ziyanli-amazon ziyanli-amazon merged commit da480fb into awslabs:master Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants