Allow setting the service name of Quadlet .pod units #23427
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
force-pushed
the
quadlet-pod-service-name
branch
from
7e48af6 to
f3a8626
Compare
| @@ -788,229 +793,229 @@ BOGUS=foo | |||
|
|
|||
| DescribeTable("Running quadlet test case", | |||
| runQuadletTestCase, | |||
| Entry("Basic container", "basic.container", 0, ""), | |||
There was a problem hiding this comment.
this diff is gigantic, is ther eno better wya to do that? i.e. wrapper function or new test case
There was a problem hiding this comment.
I put it in a separate commit in order to make it more readable. I guess it didn't work :).
Originally, I thought I'd add this feature to other unit types, but, I'm not sure it's really needed for the units that are intended to be used as dependencies.
I can look into separating this test if you think it's better
There was a problem hiding this comment.
sure a separate commit helps but the point you still touch a lot of lines for no good reason, duplicating ...0, "", ""), is annoying to see and makes it harder to read.
So thinking about this there should really be a way to pass optional arguments. Because the next time someone adds a new arg I don't want to see such a bug diff again.
There was a problem hiding this comment.
I've opened a PR to split the tables so that they can use default values: #23443. Once it's merged, I'll rebase this one
| @@ -947,6 +948,14 @@ When using `host` networking via `Network=host`, the `PublishPort=` option canno | |||
|
|
|||
| This key can be listed multiple times. | |||
|
|
|||
|
|
|||
| ### `ServiceName=` | |||
There was a problem hiding this comment.
I have nothing against setting a a specific service name but IMO this should not be added for just pods. This is totally inconsistent like that and if we do it then it should be done for all units IMO
There was a problem hiding this comment.
I thought about it as well. But then I thought:
- The service for
.containerand.kubefiles keeps the name - Unit types
.network,.volume,.imageand.buildare intended mostly for dependencies .podare the only ones that both the users interact with and the name is not kept.
Plus, making these change for other types is more complicated because it requires handling the order differently. Something along the line of what I already did for .pod files. So, this gives a quick response for the issue that was raised
There was a problem hiding this comment.
I guess I am open to wait for user feedback on this, but generally I find doing this for one option inconsistent and I am pretty sure that someone will request this for other types as well.
(I do agree that kube and container units are fine without it as they do not append anything)
|
LGTM |
|
@Luap99 Are you good to merge, or would you like to see the test changes happen in this PR? |
|
No please merge #23443 first then this can rebased which makes it much cleaner. |
|
/lgtm |
|
/hold Please do not merge it before the test cleanup |
openshift-ci
bot
added
the
do-not-merge/hold
stale-locking-app
bot
added
the
locked - please file new issue/PR
Does this PR introduce a user-facing change?
Yes
Resolves: #23414