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 extraneous newlines after warnings without breaking ansible runner #68375

Closed
wants to merge 1 commit into from

Conversation

ryanpetrello
Copy link
Contributor

@ryanpetrello ryanpetrello commented Mar 20, 2020

original PR: #65199

I'm not totally sure why, but this change appears to have introduced a regression with ansible runner's display plugin:

ansible/awx#6357

@ryanpetrello
Copy link
Contributor Author

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. has_issue needs_triage Needs a first human triage before being processed. 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 20, 2020
@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 23, 2020

@samdoran spoke with me and noted that he'd prefer not to roll back this change, as it addresses a bug. So if I find time, I may try to find another way to address this without regressing on the bug.

@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 23, 2020

I'm going to see if I can come up with a way to address the ansible bug, but also fix AWX output.

If I can't figure it out, I might need some help from somebody who's more familiar w/ Ansible's callback plugin interface, because this change is making all AWX output look like this (ansible/awx#6357):

image

@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 23, 2020

Also affects plain Ansible Runner w/ ansible 2.9.6:

image

Problem goes away if I downgrade to 2.9.0:

image

@AlanCoding
Copy link
Member

I think best place to start would be writing integration tests in ansible-runner, get fastest feedback that way. There's a lot I need to do there, but I haven't gotten back around to it.

@ryanpetrello ryanpetrello changed the title Revert "display - remove extra new line after warning message (#65199)" fix extraneous newlines after warnings without breaking ansible runner Mar 23, 2020
@ryanpetrello
Copy link
Contributor Author

ryanpetrello commented Mar 23, 2020

@samdoran I've pushed up a change that attempts to resolve this by just removing the \n from the warning method instead of moving the color code around like your original PR.

image

@ansibot ansibot removed the support:community This issue/PR relates to code supported by the Ansible community. label Mar 23, 2020
@jctanner
Copy link
Contributor

We're actively testing runner on every commit. What are our tests missing that would have caught this issue?

https://github.com/ansible/ansible/tree/devel/test/integration/targets/ansible-runner/tasks

needs_info

@ansibot ansibot added the needs_info This issue requires further information. Please answer any outstanding questions. label Mar 24, 2020
@ryanpetrello
Copy link
Contributor Author

replaced by #68517

@ansibot ansibot removed the needs_info This issue requires further information. Please answer any outstanding questions. label Mar 27, 2020
@bcoca
Copy link
Member

bcoca commented Mar 27, 2020

closing in favor of #68517

@bcoca bcoca closed this Mar 27, 2020
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Mar 30, 2020
@ansible ansible locked and limited conversation to collaborators Apr 24, 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 has_issue needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. 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

7 participants