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

flatpak: Open subprocess in text stream mode #51371

Closed
wants to merge 1 commit into from
Closed

flatpak: Open subprocess in text stream mode #51371

wants to merge 1 commit into from

Conversation

subpop
Copy link

@subpop subpop commented Jan 27, 2019

This sets the return values to be 'str' instead of byte-arrays. Fixes what appears to be a python 3 compatibility problem.

SUMMARY

This sets the return values to be 'str' instead of byte-arrays. Fixes what appears to be a python 3 compatibility problem.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

flatpak

This sets the return values to be 'str' instead of byte-arrays. Fixes what appears to be a python 3 compatibility problem.
@ansibot
Copy link
Contributor

ansibot commented Jan 27, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. python3 small_patch support:community This issue/PR relates to code supported by the Ansible community. labels Jan 27, 2019
@oolongbrothers
Copy link
Contributor

oolongbrothers commented Jan 30, 2019

Thanks for your contribution, @subpop !

I was recently working on Python 3 compatibility issues and added byte-array <> string conversions in other places of the module. Right now my integration tests are running successfully in both Python 2 and 3 on Fedora 28, Ubuntu 16.04 and 18.04 on latest devel (after applying the fixes from #51482). Quite possible that my tests do not cover your issue, though.

I would like to prevent your issue from regressing in future, so I would really like to have it covered by the integration tests. Could you add a test that triggers your issue to the flatpak integration test playbook at test/integration/targets/flatpak/tasks/test.yml in your branch?

I tried to document how to run the integration tests in the header of the flatpak.py file. If you haven't done any ansible module integration testing before, you might want to check out: https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html
If you should get stuck with creating the test I would be happy to help out 🙂

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 30, 2019
@oolongbrothers
Copy link
Contributor

oolongbrothers commented Jan 30, 2019

Actually, I just tried to apply your fix and I did get errors that the text-parameter is not supported. At least on Fedora 28 with Python 3.6.
Which Python version are you on when you experience your issue? And what version of flatpak (flatpak --version) are you runnning?

@subpop
Copy link
Author

subpop commented Jan 31, 2019

Which Python version are you on when you experience your issue? And what version of flatpak (flatpak --version) are you runnning?

I'm running Fedora 29.

ansible 2.7.5
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/link/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.7.2 (default, Jan 16 2019, 19:49:22) [GCC 8.2.1 20181215 (Red Hat 8.2.1-6)]
Flatpak 1.0.6

I'll see if I can make a test to cover this, yea.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 8, 2019
@ansibot ansibot added the packaging Packaging category label Feb 16, 2019
@gundalow gundalow changed the title Open subprocess in text stream mode flatpak: Open subprocess in text stream mode Feb 21, 2019
@oolongbrothers
Copy link
Contributor

I have a Fedora 29 environment with Python 2 running now, but I'm still not able to reproduce this bug.
@subpop What tasks are you executing when you get that behaviour? Also vital: What version of ansible are you running? 2.7 or are you running from the devel branch?

@ansibot ansibot added the needs_repo This PR no longer has an associated branch as it was removed by the submitter. label Jul 2, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 31, 2019

@subpop Your branch does not contain a shippable.yml file. Please rebase your branch to trigger running of current tests.

click here for bot help

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_shippable This PR lacks a shippable.yml in its tree. Please rebase your branch to include the latest CI tests. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 31, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 31, 2019

@subpop Your branch does not contain a shippable.yml file. Please rebase your branch to trigger running of current tests.

click here for bot help

@gundalow
Copy link
Contributor

gundalow commented Sep 3, 2019

@subpop Hi, Are you still seeing this issue in Ansible 2.8 or 2.9?

@oolongbrothers I wonder if this should be closed?

needs_info

@gundalow gundalow added the pr_day Has been reviewed during a PR review Day label Sep 3, 2019
@ansibot ansibot added the needs_info This issue requires further information. Please answer any outstanding questions. label Sep 3, 2019
@gundalow
Copy link
Contributor

Given I have a Fedora 29 environment with Python 2 running now, but I'm still not able to reproduce this bug. I'm going to close this.

Please open a new issue/PR if this is still an issue with Ansible 2.9 (or devel)

@gundalow gundalow closed this Sep 19, 2019
@ansible ansible locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. has_issue module This issue/PR relates to a module. needs_info This issue requires further information. Please answer any outstanding questions. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_repo This PR no longer has an associated branch as it was removed by the submitter. needs_shippable This PR lacks a shippable.yml in its tree. Please rebase your branch to include the latest CI tests. new_contributor This PR is the first contribution by a new community member. packaging Packaging category pr_day Has been reviewed during a PR review Day python3 small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants