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_remote: Fixing out of index error #51721

Closed
wants to merge 1 commit into
base: devel
from

Conversation

@JayKayy
Copy link

JayKayy commented Feb 5, 2019

SUMMARY

Looks like the flatpak command output changed from using spaces to tabs. changing the split() method to be performed on '\t' instead of spaces fixed the issue for me.
Fixes #51481

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

flatpak_remote

ADDITIONAL INFORMATION

Adding a remote as of late with no other remotes present would produce a IndexError on recent versions of flatpak.

Before:

jkwiatko@localhost ~/programming/ansible
$ hacking/test-module  -m lib/ansible/modules/packaging/os/flatpak_remote.py -a "name=flathub flatpakrepo_url=https://dl.flathub.org/repo/flathub.flatpakrepo
state=present"
* including generated source, if any, saving to: /home/jkwiatko/.ansible_module_generated                                                                    
* ansiballz module detected; extracted module source to: /home/jkwiatko/debug_dir                                                                            
***********************************
RAW OUTPUT

Traceback (most recent call last):
  File "/home/jkwiatko/debug_dir/__main__.py", line 241, in <module>
    main()
  File "/home/jkwiatko/debug_dir/__main__.py", line 230, in main
    remote_already_exists = remote_exists(module, binary, to_bytes(name), method)                                                                            
  File "/home/jkwiatko/debug_dir/__main__.py", line 172, in remote_exists
    if listed_remote[0] == to_native(name):
IndexError: list index out of range

***********************************
INVALID OUTPUT FORMAT

Traceback (most recent call last):
  File "hacking/test-module", line 223, in runtest
    results = json.loads(out)
  File "/usr/lib64/python2.7/json/__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python2.7/json/decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib64/python2.7/json/decoder.py", line 382, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded

After:

$ hacking/test-module  -m lib/ansible/modules/packaging/os/flatpak_remote.py -a "name=flathub flatpakrepo_url=https://dl.flathub.org/repo/flathub.flatpakrepo
state=present"
* including generated source, if any, saving to: /home/jkwiatko/.ansible_module_generated                                                                    
* ansiballz module detected; extracted module source to: /home/jkwiatko/debug_dir                                                                            
***********************************
RAW OUTPUT

{"stdout": "", "changed": true, "command": "/usr/bin/flatpak remote-add --system flathub https://dl.flathub.org/repo/flathub.flatpakrepo", "stderr": "", "rc": 0, "invocation": {"module_args": {"method": "system", "executable": "flatpak", "name": "flathub", "flatpakrepo_url": "https://dl.flathub.org/repo/flathub.flatpakrepo", "state": "present"}}}


***********************************
PARSED OUTPUT
{
    "changed": true,
    "command": "/usr/bin/flatpak remote-add --system flathub https://dl.flathub.org/repo/flathub.flatpakrepo",                                               
    "invocation": {
        "module_args": {
            "executable": "flatpak",
            "flatpakrepo_url": "https://dl.flathub.org/repo/flathub.flatpakrepo",                                                                            
            "method": "system",
            "name": "flathub",
            "state": "present"
        }
    },
    "rc": 0,
    "stderr": "",
    "stdout": ""
}
$ flatpak remote-list
Name    Options
flathub system
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 5, 2019

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Feb 18, 2019

Depending on the version of flatpak that's available do we need to support both tabs and spaces?

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Feb 20, 2019

@gundalow Yes. The breakage is easily visible when running the integration tests against both Fedora 28 and Ubuntu 18.04.
We need to continue supporting both output types, since we don't want to drop support for flatpak < 1.2.0.
I had created #51482 on Jan 30th that solves this issue while keeping compatibility by detecting the flatpak version and then using different parsing logic.
Edit: To be more precise: In the PR, the code checks if the --columns feature is present in the used flatpak executable version. If it is, it uses that, since it simplifies the parsing greatly and makes it less prone to future breakage because the default output formatting would have changed. If the --columns
feature is absent, we use the old logic.

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Feb 20, 2019

@JayKayy Thanks for looking at this :) Happy to see that you're still having an eye on the modules.
Sorry that the way how I created a bug report and then a PR directly after lead to confusion.

@gundalow gundalow changed the title Fixing out of index error flatpak_remote: Fixing out of index error Feb 21, 2019

@@ -168,7 +168,7 @@ def remote_exists(module, binary, name, method):
# The query operation for the remote needs to be run even in check mode
output = _flatpak_command(module, False, command)
for line in output.splitlines():
listed_remote = line.split()
listed_remote = line.split('\t')

This comment has been minimized.

@bmalynovytch

bmalynovytch Feb 21, 2019

As asked by @gundalow, to avoid breaking compatibility with old releases, you should probably split on either space or tabs.

This comment has been minimized.

@oolongbrothers

oolongbrothers Feb 21, 2019

Contributor

@bmalynovytch Wouldn't the simple approach of supporting tabs and spaces potentially lead to more breakage in future when flatpak again decides to change their output format in some subtle way? I mean the reason why I implemented it this way originally was only because there was no better way to get output from the flatpak binary.
From version 1.2 (the one that breaks with the current code), however, there now is a parameter called --columns that allows us to control the output of the list-remote command. That way we should be more future-proof by specifying --columns=name in case of flatpak >= 1.2.
I implemented this behaviour for the flatpak module in PR #51482 (still unmerged). Please have a look and comment there if you have any views. I'm not religious about my solution. If you guys think it's too complex, we could go for a simpler approach. Maybe it's not worth it.
But what I do feel strongly about is to implement the behaviour the same way among the flatpak and the flatpak_remote modules, for consistency.

This comment has been minimized.

@bmalynovytch

bmalynovytch Feb 21, 2019

In fact, the good question is: with your contrib, do older versions still work or will it break compatibility ? (ie. Ansible 2.8 + Flatpak < 1.2.0)
If so, we need to be able to handle the situation (like you did in #51482)

This comment has been minimized.

@oolongbrothers

oolongbrothers Feb 21, 2019

Contributor

I ran the integration tests on this PR and they confirm that the code above works for both flatpak 1.2.0 and before.
I mean, I like the fact that @JayKayy's solution is super simple.
Even if it is possible that flatpak breaks it's output in future again, we don't know if they actually will. So maybe one could regard a solution like mine from #51482 as premature optimisation...

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Feb 21, 2019

I had created #51482 on Jan 30th that solves this issue while keeping compatibility by detecting the flatpak version and then using different parsing logic.

By the way, want to I apologise for the above comment, @JayKayy, @gundalow
That is not true: I implemented this behaviour for the flatpak module not the flatpak_remote module. I got confused there, sorry.

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Feb 21, 2019

The breakage is easily visible when running the integration tests against both Fedora 28 and Ubuntu 18.04.

Uhm, I need to apologise again. When I ran the integration tests again now, they clearly showed that the code works for both old and new versions of flatpak. Dunno what I did last time, must have run against the wrong branch. Sorry, especially to you, @JayKayy

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Feb 22, 2019

I had created #51482 on Jan 30th that solves this issue while keeping compatibility by detecting the flatpak version and then using different parsing logic.

By the way, want to I apologise for the above comment, @JayKayy, @gundalow
That is not true: I implemented this behaviour for the flatpak module not the flatpak_remote module. I got confused there, sorry.

It's all good. Nice to see lots of activity on the various flatpak modules :)

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Mar 10, 2019

@JayKayy Could you maybe rebase this branch, so it can be merged. I think we should try to get this out soon 🙂

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Mar 20, 2019

@gundalow When @JayKayy is off the radar, it can take quite some time before he comes back. What I could do is to create another PR that applies his fix to the current state of devel. Could accelerate things, if you could help to get that one merged quickly then.

@oolongbrothers

This comment has been minimized.

Copy link
Contributor

oolongbrothers commented Mar 20, 2019

According to the latest findings from #51481, this bug only affects 2.7, since a fix for this was merged to devel earlier.

I created backport PR #54103 applying that fix to 2.7.

That is why I would like to request to close this PR.

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Mar 20, 2019

OK, given the above I'll close this.

1 similar comment
@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Mar 20, 2019

OK, given the above I'll close this.

@gundalow gundalow closed this Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.