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

Accept trailing commas in list config type #80908

Open
1 task done
sjshuck opened this issue May 27, 2023 · 10 comments
Open
1 task done

Accept trailing commas in list config type #80908

sjshuck opened this issue May 27, 2023 · 10 comments
Labels
affects_2.16 feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. waiting_on_contributor This would be accepted but there are no plans to actively work on it.

Comments

@sjshuck
Copy link

sjshuck commented May 27, 2023

Summary

It's not documented exactly what delimits a list. In the code, it's commas:

value = [unquote(x.strip()) for x in value.split(',')]

But trailing commas are not supported, because they introduce spurious empty string elements. It would be nice, since the INI-style config file format accepts multiline values de facto.

And of course, document all the above.

Issue Type

Feature Idea

Component Name

lib/ansible/config/manager.py

Additional Information

I'm preparing a PR for this. But first, please see #80907.

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@ansibot
Copy link
Contributor

ansibot commented May 27, 2023

Files identified in the description:

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot ansibot added affects_2.16 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. labels May 27, 2023
@bcoca
Copy link
Member

bcoca commented May 30, 2023

this has an issue with the ability to add empty items to a list, specially if it is the last one

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label May 30, 2023
@sjshuck
Copy link
Author

sjshuck commented May 30, 2023

@bcoca Not if implemented correctly. I'll show the code I have in mind but I still would like #80907 addressed before I prepare a full PR with docs changes etc.

@sjshuck
Copy link
Author

sjshuck commented May 30, 2023

sjshuck@3cee653

You can have empty quotes ("", '') as a list element and it won't get filtered out. (Not so for pathlist, but an empty pathlist item doesn't make sense since paths cannot be empty.)

@bcoca
Copy link
Member

bcoca commented May 30, 2023

empty path == relative path == cwd (current working directory)

@sjshuck
Copy link
Author

sjshuck commented May 30, 2023

I'm sure you know that . is the standard way to represent cwd, so I guess I don't know what you're saying. Are you suggesting someone might interpret an empty path that way in theory, or are you saying Ansible and/or some modules interpret it that way in practice?

Either way, no file API or OS I'm aware of considers the empty string to be a legal path, whether absolute or relative.

@sivel
Copy link
Member

sivel commented May 30, 2023

Python specifically is one of those things where an empty path effectively means cwd.

>>> import os
>>> os.path.abspath('')
'/home/sivel'

As such pretty much all of our code, including, but not limited to, resolve_path and unfrackpath works the same way. This makes the expectation of using an empty string as the cwd a reality.

We handle any cases where an empty string doesn't meet expectations on a case by case basis, and not on a global basis.

@sjshuck
Copy link
Author

sjshuck commented May 30, 2023

I didn't know that about Python. Thanks for explaining @sivel.

I was going to suggest that pathlist also unquote() elements, just like list does, but again that discussion probably belongs in #80907.

@sivel
Copy link
Member

sivel commented May 30, 2023

I should also note, that in our base.yml only 1 item uses pathlist (DEFAULT_HOST_LIST), and it's completely legal to supply an empty string there.

Also, for values supplied in ansible.cfg they are relative to the ansible.cfg, so an empty string there is equivalent to dirname of the ansible.cfg file.

@sjshuck
Copy link
Author

sjshuck commented May 30, 2023

So one of my goals in this and the other issue is to clearly document what the format is, even if it doesn't change.

Right now, you have things like this. It says type: list, but it isn't explained anywhere what that actually means. The default is surrounded with [...] which AFAICT from the code (I didn't test) would choke on those characters. Other options don't have those characters. I think it's possible to specify the list type in documentation in a succinct, friendly way.

@bcoca bcoca added the waiting_on_contributor This would be accepted but there are no plans to actively work on it. label Jan 3, 2024
@ansibot ansibot added the has_pr This issue has an associated PR. label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.16 feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. waiting_on_contributor This would be accepted but there are no plans to actively work on it.
Projects
None yet
Development

No branches or pull requests

4 participants