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-981: Restore optional usage of chatId Configuration Option and correct narrative #1318

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

manstis
Copy link

@manstis manstis commented Feb 28, 2023

Following discussions with @valdar it was felt that including a default value and improving the documentation was the best approach. Keeping the parameter as required forces Users to explicitly consider how/where to set the value. Reverting optional behaviour can lead to implicit failures that can be more difficult to diagnose.

@valdar @oscerd WDYT?

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

I don't think default should be set

type: string
default: dummy
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set a default value, it will be set on the endpoint by default.

Copy link
Author

@manstis manstis Feb 28, 2023

Choose a reason for hiding this comment

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

This is @valdar's preference.

However I see your point too. Damn. Why do I always see both sides to things!?!?

The User is forced to either provide a real value for the endpoint configuration or set its value in the header where it then takes precedence over the endpoint configuration. The default value is explicitly mentioned in the updated documentation and advises it could lead to delivery failure.

It's a difficult situation: The parameter is required, it is not optional; but where it can be set (endpoint or per message) means Camel fails to deploy the Kamelet if the User does not set a value in the Configuration Options when marked as required.

We want to somehow impose configuration of a chatId on Users.

I'll chat to @valdar again..

Copy link
Author

Choose a reason for hiding this comment

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

OK. I've restored the required nature of the chatId parameter and hopefully made the documentation bullet proof!

@manstis manstis requested a review from oscerd February 28, 2023 15:05
@manstis manstis marked this pull request as ready for review February 28, 2023 15:09
@manstis
Copy link
Author

manstis commented Feb 28, 2023

Assuming this is acceptable; please confirm you'll want the other affected branches (3.x and 3.20.x) corrected too.

@oscerd oscerd merged commit 7fde910 into apache:main Feb 28, 2023
@oscerd
Copy link
Contributor

oscerd commented Feb 28, 2023

This should also be backported on 3.x and 3.20.x

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

2 participants