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

Fix payload to match Rocket Chat 7.4.1 API #9882

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

broferek
Copy link

SUMMARY

This PR fixes the following error with Rocket Chat 7.4.1

"error":"Unexpected token \'p\\', \\\\"payload{\\\\\"t\\\\\"... is not valid JSON"
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Rocket Chat

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review labels Mar 14, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Mar 14, 2025
@felixfontein
Copy link
Collaborator

Thanks for your contribution! This is a duplicate of #9869 (and has the same problems I mentioned in #9869 (comment)).

@felixfontein
Copy link
Collaborator

See also #9869 (comment).

@ansibullbot

This comment was marked as resolved.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 14, 2025
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI small_patch Hopefully easy to review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 14, 2025
@russoz
Copy link
Collaborator

russoz commented Mar 16, 2025

Hi @broferek ,

I reckon one way to avoid the request duplication is to create a feature flag - a new parameter to the module, it could be a bool, indicating whether the module should behave as pre-7.4.0 or post-7.4.0. Set the default to pre-7.4.0 to keep backward compatibility but deprecate that default value so that users will know they need to use the post-7.4.0 setting.

@russoz
Copy link
Collaborator

russoz commented Mar 16, 2025

Also, please add a changelog fragment.

broferek pushed a commit to broferek/community.general that referenced this pull request Mar 18, 2025
@ansibullbot

This comment was marked as resolved.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 18, 2025
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Mar 18, 2025
@broferek
Copy link
Author

broferek commented Mar 18, 2025

Hi @russoz See my latest commits. I tried to follow your suggestions.

@felixfontein felixfontein removed the backport-9 Automatically create a backport for the stable-9 branch label Mar 18, 2025
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @broferek, thanks for the updates! Got a couple of adjustments and suggestions.

@broferek
Copy link
Author

Hi @russoz, I think I got everything right this time. Thanks to review it.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Minor nit in the doc block, other than that LGTM

@broferek
Copy link
Author

@russoz Thanks for your feedback. The missing punctuation has been fixed. I guess we are good to merge now.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Sorry for not having managed to read through this earlier, I also have some docs suggestions :)

@@ -0,0 +1,2 @@
minor_changes:
- rocketchat - parameter ``option_is_pre740`` has been added to control the format of the old payload (https://github.com/ansible-collections/community.general/pull/9882)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- rocketchat - parameter ``option_is_pre740`` has been added to control the format of the old payload (https://github.com/ansible-collections/community.general/pull/9882)
- rocketchat - option ``is_pre740`` has been added to control the format of the payload. For Rocket.Chat 7.4.0 or newer, it must be set to ``false`` (https://github.com/ansible-collections/community.general/pull/9882).

description:
- If V(true), the payload matches Rocket Chat prior to 7.4.0 format.
- The default value of the parameter can be set to false in a few months' time.
- This parameter will be removed in a future release when Rocket Chat 7.4.0 becomes the minimum supported version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the official spelling is Rocket.Chat with a period:

Suggested change
- This parameter will be removed in a future release when Rocket Chat 7.4.0 becomes the minimum supported version.
- This parameter will be removed in a future release when Rocket.Chat 7.4.0 becomes the minimum supported version.

(Also it might be that this has been already working with Rocket.Chat for a long time, and it was just that they dropped support for the old way in 7.4.0 - maybe because they thought that nobody is using it anymore since forever.)

is_pre740:
description:
- If V(true), the payload matches Rocket Chat prior to 7.4.0 format.
- The default value of the parameter can be set to false in a few months' time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- The default value of the parameter can be set to false in a few months' time.
- The default value of the option will change to V(false) eventually.

@@ -100,6 +100,14 @@
elements: dict
description:
- Define a list of attachments.
is_pre740:
description:
- If V(true), the payload matches Rocket Chat prior to 7.4.0 format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- If V(true), the payload matches Rocket Chat prior to 7.4.0 format.
- If V(true), the payload matches Rocket.Chat prior to 7.4.0 format. This format has been used by the module since its inception, but is no longer supported by Rocket.Chat 7.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants