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

CAMEL-21352: extend camel smb #16686

Merged
merged 8 commits into from
Jan 3, 2025

Conversation

jonomorris
Copy link
Contributor

Description

Expand the options available to the SMB consumer and producer by extending GenericFile classes.

A number of additional consumer and producer options are now supported including 'move', 'preMove', 'moveFailed', 'readLock', 'readLockTimeout', 'delete', 'include', 'exclude', 'includeExt', 'excludeExt', 'filter', 'localWorkDirectory', 'sortBy', 'maxMessagesPerPoll', 'maxDepth', 'minDepth', 'sorter', 'maxMessagesPerPoll', 'eagerMaxMessagesPerPoll', 'idempotentKey', and more.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

Copy link
Contributor

github-actions bot commented Jan 2, 2025

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@jonomorris
Copy link
Contributor Author

jonomorris commented Jan 2, 2025

/component-test smb

Result ❌ The tests failed please check the logs

Copy link
Contributor

github-actions bot commented Jan 2, 2025

🤖 The Apache Camel test robot will run the tests for you 👍

@davsclaus
Copy link
Contributor

davsclaus commented Jan 2, 2025

/component-test camel-smb

Result ✅ The tests passed successfully

Copy link
Contributor

github-actions bot commented Jan 2, 2025

🤖 The Apache Camel test robot will run the tests for you 👍

@orpiske
Copy link
Contributor

orpiske commented Jan 3, 2025

I am curious about why adding an entirely new component instead of refactoring the existing one.

Copy link
Contributor

@orpiske orpiske left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, but I am curious about the point I added above (i.e: adding a new component).

IMHO, Camel is already far too bloated and, if we could adjust what we have, that would be better.

@davsclaus
Copy link
Contributor

This is not the final work. We will delete the old and rename to smb when its ready.
If this is already the case then @jonomorris you are welcome to do that.

@orpiske
Copy link
Contributor

orpiske commented Jan 3, 2025

This is not the final work. We will delete the old and rename to smb when its ready. If this is already the case then @jonomorris you are welcome to do that.

Ah, got it, thanks for the explanation!

That sounds OK to me, but let's make sure we do it in one step so not to mess the git history (i.e.: so we can easily trace back to its origins, changes, and bisect things).

@orpiske
Copy link
Contributor

orpiske commented Jan 3, 2025

This is not the final work. We will delete the old and rename to smb when its ready. If this is already the case then @jonomorris you are welcome to do that.

Ah, got it, thanks for the explanation!

That sounds OK to me, but let's make sure we do it in one step so not to mess the git history (i.e.: so we can easily trace back to its origins, changes, and bisect things).

Actually, forget that. It's not that old and so not much to lose. Feel free to do it as you seem fit.

@jonomorris
Copy link
Contributor Author

@davsclaus I'd be happy to delete the old and rename to smb. Would you like that done in this PR?

@davsclaus
Copy link
Contributor

@davsclaus I'd be happy to delete the old and rename to smb. Would you like that done in this PR?

Yes that would be good

@github-actions github-actions bot removed the core label Jan 3, 2025
@github-actions github-actions bot removed the docs label Jan 3, 2025
@jonomorris
Copy link
Contributor Author

jonomorris commented Jan 3, 2025

/component-test camel-smb

Result ✅ The tests passed successfully

Copy link
Contributor

github-actions bot commented Jan 3, 2025

🤖 The Apache Camel test robot will run the tests for you 👍

@jonomorris
Copy link
Contributor Author

@davsclaus I've moved smb2 to smb now. Thanks.

@davsclaus
Copy link
Contributor

okay so its ready to merge

@davsclaus davsclaus merged commit 550cc65 into apache:smb2 Jan 3, 2025
2 checks passed
@davsclaus
Copy link
Contributor

okay I am going to merge this over to main, and regen CSB

@davsclaus
Copy link
Contributor

@jonomorris we need to update the 4.10 upgrade guide about camel-smb is refactored to allow users to be aware.

davsclaus pushed a commit that referenced this pull request Jan 3, 2025
* CAMEL-21352 camel-smb should extend from camel-file to offer more features
@davsclaus
Copy link
Contributor

@jonomorris its merged to main now, great work, and a lot of tests.

I have these errors locally, not sure if you see that too

[ERROR] Failures:
[ERROR]   SmbConsumerPartialReadNoPathIT.testSmbSimpleConsumeAbsolute mock://result Body of message: 0. Expected: <Hello World> but was: <24483
>
[ERROR]   SmbRecursiveMaxDepthIT.testDirectoryTraversalDepth mock://received_send Body of message: 0. Expected: <Goodday> but was: <World>
[ERROR]   SmbRecursiveMinDepthIT.testDirectoryTraversalDepth mock://received_send Body of message: 0. Expected: <Hello> but was: <Goodday>
[INFO]
[ERROR] Tests run: 58, Failures: 3, Errors: 0, Skipped: 0

@jonomorris
Copy link
Contributor Author

@davsclaus that's great, thanks. I'll review those test cases, they pass locally. Will also add some notes to the upgrade guide.

@jonomorris jonomorris deleted the CAMEL-21352_extend_camel-smb branch January 4, 2025 10:11
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.

4 participants