Strip key/values of external quotes

[rhatdan]

Does this PR introduce a user-facing change?

quadlet will strips quotes around values in quadlets.

Fixes: #20992

[ygalblum]

I also looked at this issue. The problem is that some keys do require keeping the double quotes. See the hostname.container test which fails.

[rhatdan]

I was wondering that.

[rhatdan]

After looking at the failures, I think the new code is correct, stripping the quotes. I believe this matches better with systemd.

[ygalblum]

Suggested change

	## assert-podman-final-args localhost/imagename
	## assert-podman-args "RemoveQuotesName"
	[Container]
	Image="localhost/imagename"
	Name="RemoveQuotesName"
	## assert-podman-final-args localhost/imagename
	## assert-podman-args "--name=RemoveQuotes\"Name"
	[Container]
	Image="localhost/imagename"
	ContainerName="RemoveQuotes"Name"

[ygalblum]

After looking at the failures, I think the new code is correct, stripping the quotes. I believe this matches better with systemd.

You're right. Not sure why, but when I ran it yesterday without the quotes the hostname was not set. I've tested it again and it even makes more sense now. Don't know what went wrong

[Luap99]

But is striping the quotes actually the problem? I would think systemd uses some form of escape logic similar to the shell so just stripping does not sounds right. https://www.freedesktop.org/software/systemd/man/latest/systemd.syntax.html

If you look at pkg/systemd/parser/split.go there is already a bunch of code to take care of thie escaping AFAICT so I would assume you would need to use that to correctly parse values.

[rhatdan]

@Luap99 I don't see anything obvious there. Most of those look like something that splits a string into a series of arguments, similar to shlex.

[Luap99]

extractFirstWord() for example. LookupAllStrv() already handles this correctly because it calls into that.

[rhatdan]

I don't have time to work on this and it is a simple enough change, that I think we should just merge, and if you want to open an issue to create a better fix in the future lets open that.

@vrothberg @giuseppe @Luap99 @ashley-cui @mheon @baude @ygalblum @umohnani8 PTAL

[giuseppe]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[markstos]

Thanks for this fix! It seems ready to merge, although some MAc and Windows checks failed.

[mheon]

Needs a rebase to fix Windows CI, Mac is an expected failure