-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Quadlet add support for --add-host --ip and --ip6 #23713
Conversation
Signed-off-by: Jerome degroote <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. 2 small comments.
In addition, since you define the structure for the AddHost key and return errors on invalid values, please add negative test cases as well. You can find examples in this table: https://github.com/containers/podman/blob/main/test/e2e/quadlet_test.go#L1018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I like a new different syntax for only quadlet files. IMO the syntax should match podman cli as much as possible.
So if you really think this syntax is needed then it should go into podman proper so there are not random syntax differences for the option values.
pkg/systemd/quadlet/quadlet.go
Outdated
|
||
// Add hosts to the podman command | ||
// An IP can refer to one or multiple hosts following the format : | ||
// my-host-name;my-second-host-name;third-hostname:192.168.10.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of inventing a new syntax? If you want multiple hosts entries specify them several times.
I am not sure why this extra complexity is needed over the simple
addHosts := quadletUnitFile.LookupAll(groupName, KeyAddHost)
for _, addHost := range addHosts {
podman.add("--add-host=" + addHost)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an application which needs a lots of entry in the host files and multiple hostname refer to the same IP.
With kube play (or also directly by editing the hosts file), we can set multiple hostname for an IP without duplicating that IP. I think it helps with the clarity of the file.
However, I agree that it would be better if it wasn't a quadlet specific syntax. Beginning of next week I will remove this specificity from the PR.
397c4af
to
41877a6
Compare
please squash your fixup commits into the AddHost commit, having two commit for IP and AddHost is great but having extra commits that remove features/fix bug from an earlier commit in the PR should juts be squashed into the original commit adding them. |
Signed-off-by: Jerome degroote <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks, If you like to have the several host for one ip syntax I suggest you file a new issue so we can consider this for the proper podman cli. Then quadlet units could use it without further changes.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jerome59, Luap99, ygalblum 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:
Approvers can indicate their approval by writing |
Fixes: #23692
Does this PR introduce a user-facing change?