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

Issue #254 migrate smb extension repo into spring integration main repo #3791

Conversation

GregBragg
Copy link
Contributor

@GregBragg GregBragg commented May 5, 2022

@artembilan as discussed in Spring Integration Extensions repo for Issue spring-projects/spring-integration-extensions#254

@artembilan
Copy link
Member

@GregBragg ,

please, consider to rebase your branch to the latest main: there is a fix for broken API from upstream dependency.

I will look into this tomorrow then.

Thank you!

…nsion_repo_into_Spring_Integration_main_repo
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

A couple not critical, but useful pointers:

  1. Why "merge"? Why everyone does "merge", when I am clearly asking to do a rebase. Your change might be more actual, than what you pull from the main.
  2. The commit message is important, at least the first one: https://cbea.ms/git-commit/
  3. We don't use JUnit asserts any more. AssertJ is the library we rely on. I'm not sure if this PR is going to fail against our Checkstyle rules on the matter, but there is an IntelliJ IDEA plugin to migrate smoothly: https://plugins.jetbrains.com/plugin/10345-assertions2assertj
  4. All these classes are new for this project, so their @since must be 6.0. Can be done in the new PR if this is a lot of burden for the current one.
  5. The Copyright date must be updated to the current year.
  6. Did you run ./gradlew clean :spring-integration-smb:check locally?
  7. We will need a documentation for this new module. Yes, another PR would be better.

I think that's it. As far as this PR is green, I'm merging.

Thank you!

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Right, Checkstyle has just failed:

 > Task :spring-integration-smb:checkstyleTest FAILED
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/AbstractBaseTests.java:19:31: Using a static member import should be avoided - org.junit.Assert.assertNotNull. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/AbstractBaseTests.java:20:31: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/SmbMessageHistoryTests.java:19:31: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundChannelAdapterParserTests.java:19:36: Using a static member import should be avoided - org.hamcrest.Matchers.instanceOf. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundChannelAdapterParserTests.java:20:31: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundChannelAdapterParserTests.java:21:31: Using a static member import should be avoided - org.junit.Assert.assertNotNull. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundChannelAdapterParserTests.java:22:31: Using a static member import should be avoided - org.junit.Assert.assertThat. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundChannelAdapterParserTests.java:23:31: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbInboundOutboundSample.java:19:31: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbOutboundChannelAdapterParserTests.java:19:31: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbOutboundChannelAdapterParserTests.java:20:31: Using a static member import should be avoided - org.junit.Assert.assertNotNull. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbOutboundChannelAdapterParserTests.java:21:31: Using a static member import should be avoided - org.junit.Assert.assertSame. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/config/SmbOutboundChannelAdapterParserTests.java:22:31: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/session/SmbSessionFactoryWithCIFSContextTests.java:20:31: Using a static member import should be avoided - org.junit.Assert.assertNotNull. [AvoidStaticImport]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-smb/src/test/java/org/springframework/integration/smb/session/SmbSessionTests.java:19:31: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]

Please, consider to migrate to AssertJ as I explained before.

@GregBragg
Copy link
Contributor Author

Hi Artem, Thanks for the review.

  1. Sorry I usually do a merge up when we do development here at CMIG, we don't do rebase normally, however I will make note of that for the next time
  2. I will be more verbose for the next time
  3. I copied the code as is from the extensions library and it built fine locally so did not get notified that it didn't follow the checkstyle rules, I also use Eclipse for development.
  4. Ok I can change this
  5. Ok I can change this
  6. No I did not originally, I did now and I see the failures
  7. Yeah I was wondering about that... Where does the documentation go and I'm assuming in the format for the original ReadMe from the extensions project?

@artembilan
Copy link
Member

You can find docs in the spring-integration\src\reference\asciidoc dir.
So, we would need to create an smb.adoc file and include it into the index.adoc and index-single.adoc.
Then mention as a New Component in the whats-new.adoc.
In the end it also would be great to add SMB channel adapters into the endpoint-summary.adoc.

But as I said: separate PR and so on.

@GregBragg
Copy link
Contributor Author

Ok np, working on the changes now for AssertJ and the others... Thanks!

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

LGTM

Pulling locally for final review, clean up and merge.

@artembilan
Copy link
Member

Merged as 7ad71d3.

@GregBragg ,

thank you again for contribution; feel free to go ahead with the docs PR!

@artembilan artembilan closed this May 6, 2022
@artembilan artembilan added this to the 6.0 M3 milestone May 6, 2022
@GregBragg GregBragg deleted the Issue_#254_-Migrate_SMB_Extension_repo_into_Spring_Integration_main_repo branch September 1, 2022 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants