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

dnf: don't return fail_json['msg'] as list #65302

Merged
merged 1 commit into from Dec 4, 2019

Conversation

ianw
Copy link
Contributor

@ianw ianw commented Nov 27, 2019

SUMMARY

Although it's not enforced that fail_json['msg'] should be a string
[1], I think it is pretty strongly implied.

In this case the failure_response['msg'] is sent through as the
fail_json['msg'], and the trailing commas here turn it into a tuple.
It's not clear if this is a typo or intended, but it does cause
problems for callbacks that expect this as a string (e.g. [2]).

Since there seems no point to having these values as a tuple, remove
the trailing commas so they return a string as per everywhere else.

[1]

if 'msg' not in kwargs:

[2] https://review.opendev.org/#/c/696081

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

dnf

@ansibot
Copy link
Contributor

ansibot commented Nov 27, 2019

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. packaging Packaging category small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Nov 27, 2019
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

This bug caused serious issues on Zuul CI because msg was always a string, also its name indicates that.

I guess that coma ended up there as a typo and not on purpose, as there is nowhere any indication of tuples being expected.

@bcoca @sivel This will also need to be backported to 2.8 branch.

@ssbarnea
Copy link
Member

ssbarnea commented Dec 4, 2019

I was told by @felixfontein on irc that this bugfix needs a changelog entry file.

@ssbarnea
Copy link
Member

ssbarnea commented Dec 4, 2019

Because I cannot update this PR, I made a PR on the source branch of @ianw at ianw#1 -- if a core can cherry pick it from there even better.

@maxamillion
Copy link
Contributor

needs changelog fragment, but otherwise it's good

@maxamillion
Copy link
Contributor

shipit

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Dec 4, 2019
@felixfontein
Copy link
Contributor

@ianw could you merge @ssbarnea's PR on your branch, or add a (similar) changelog fragment yourself?

(If you merge the PR: make sure to either choose "Rebase and merge" or "Squash and merge" (via the arrow next to the big green merge button); the default merge will create a merge commit, which will cause ansibot to scream at you :) )

Although it's not enforced that fail_json['msg'] should be a string
[1], I think it is pretty strongly implied.

In this case the failure_response['msg'] is sent through as the
fail_json['msg'], and the trailing commas here turn it into a tuple.
It's not clear if this is a typo or intended, but it does cause
problems for callbacks that expect this as a string (e.g. [2]).

Since there seems no point to having these values as a tuple, remove
the trailing commas so they return a string as per everywhere else.

[1] https://github.com/ansible/ansible/blob/4c589661c2add45023f2bff9203e0c4e11efe5f6/lib/ansible/module_utils/basic.py#L2078
[2] https://review.opendev.org/#/c/696081
@ianw
Copy link
Contributor Author

ianw commented Dec 4, 2019

@ianw could you merge @ssbarnea's PR on your branch, or add a (similar) changelog fragment yourself?

@felixfontein @ssbarnea done with the latest changeset, thanks

@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed small_patch labels Dec 4, 2019
@maxamillion
Copy link
Contributor

rebuild_merge

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Dec 4, 2019
@ansibot ansibot merged commit fc54ae9 into ansible:devel Dec 4, 2019
@felixfontein
Copy link
Contributor

Awesome! Who wants to create a backport PR?

@ssbarnea
Copy link
Member

ssbarnea commented Dec 5, 2019

ssbarnea pushed a commit to ssbarnea/ansible that referenced this pull request Dec 5, 2019
Although it's not enforced that fail_json['msg'] should be a string
[1], I think it is pretty strongly implied.

In this case the failure_response['msg'] is sent through as the
fail_json['msg'], and the trailing commas here turn it into a tuple.
It's not clear if this is a typo or intended, but it does cause
problems for callbacks that expect this as a string (e.g. [2]).

Since there seems no point to having these values as a tuple, remove
the trailing commas so they return a string as per everywhere else.

[1] https://github.com/ansible/ansible/blob/4c589661c2add45023f2bff9203e0c4e11efe5f6/lib/ansible/module_utils/basic.py#L2078
[2] https://review.opendev.org/#/c/696081

(cherry picked from commit fc54ae9)
ssbarnea pushed a commit to ssbarnea/ansible that referenced this pull request Dec 5, 2019
Although it's not enforced that fail_json['msg'] should be a string
[1], I think it is pretty strongly implied.

In this case the failure_response['msg'] is sent through as the
fail_json['msg'], and the trailing commas here turn it into a tuple.
It's not clear if this is a typo or intended, but it does cause
problems for callbacks that expect this as a string (e.g. [2]).

Since there seems no point to having these values as a tuple, remove
the trailing commas so they return a string as per everywhere else.

[1] https://github.com/ansible/ansible/blob/4c589661c2add45023f2bff9203e0c4e11efe5f6/lib/ansible/module_utils/basic.py#L2078
[2] https://review.opendev.org/#/c/696081

(cherry picked from commit fc54ae9)
anshulbehl pushed a commit to anshulbehl/ansible that referenced this pull request Dec 10, 2019
Although it's not enforced that fail_json['msg'] should be a string
[1], I think it is pretty strongly implied.

In this case the failure_response['msg'] is sent through as the
fail_json['msg'], and the trailing commas here turn it into a tuple.
It's not clear if this is a typo or intended, but it does cause
problems for callbacks that expect this as a string (e.g. [2]).

Since there seems no point to having these values as a tuple, remove
the trailing commas so they return a string as per everywhere else.

[1] https://github.com/ansible/ansible/blob/4c589661c2add45023f2bff9203e0c4e11efe5f6/lib/ansible/module_utils/basic.py#L2078
[2] https://review.opendev.org/#/c/696081
@ansible ansible locked and limited conversation to collaborators Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. module This issue/PR relates to a module. packaging Packaging category shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants