Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

[jjo] add args_format to kube.Container() #13

Closed
wants to merge 1 commit into from

Conversation

jjo
Copy link
Contributor

@jjo jjo commented Nov 17, 2018

Add args_format to allow overriding args[] generation
as per below example.

Given args_: {'k': 'v'}:
(default, current behavior) => args: ['--k=v']
args_format+: {prefix: '-'} => args: ['-k=v']
args_format+: {split: true } => args: ['--k', 'v']
args_format+: {prefix: '-', split: true } => args: ['-k', 'v']

Fixes #12.

Add `args_format` to allow overriding args[] generation
as per below example.

Given args_: {'k': 'v'}:
  (default, current behavior)               => args: ['--k=v']
  args_format+: {prefix: '-'}               => args: ['-k=v']
  args_format+: {split: true }              => args: ['--k', 'v']
  args_format+: {prefix: '-', split: true } => args: ['-k', 'v']
@anguslees
Copy link
Contributor

I'm ok with this, but I have a mild preference for optional helpers and redeclaration rather than mandatory abstraction and parameterisation. If you want to follow the redeclaration approach, the above use-cases would look like:

// Current --foo=bar
args: ["--%s=%s" % kv for kv in $.objectItems(self.args_)],

// -foo=bar
args: ["-%s=%s" % kv for kv in $.objectItems(self.args_)],

// --foo bar
args: std.flattenArrays([["--" + kv[0], kv[1]] for kv in $.objectItems(self.args_)]),

// -foo bar
args: std.flattenArrays([["-" + kv[0], kv[1]] for kv in $.objectItems(self.args_)]),

.. and it obviously generalises to all sorts of things, like foo:bar (jenkins.jsonnet example) without accumulating complexity.

Personally, I find just rewriting the list comprehension is both easier to read and write, rather than having to go and read the source of some other function to remind myself what the (eg) split parameter does / is called - but I acknowledge that I'm unusually familiar with jsonnet and others might find the parameterisation version easier to engage with. I leave it totally up to you.

@floriankoch
Copy link

@anguslees so you suggest to not using a helper at all?

@anguslees
Copy link
Contributor

anguslees commented Nov 21, 2018

so you suggest to not using a helper at all?

@floriankoch In this case, correct. But it's a personal style thing and I'm not even consistent within my own guidelines - so I'm ok with either approach in this PR.

As a general jsonnet style, I try to avoid creating helpers that result in just replacing one line with another line - preferring open cut+paste in those simple cases. Otherwise I find the temptation to add clever bits of coding everywhere is overwhelming and, before I know it, I have a jsonnet object that no longer looks anything like a regular Kubernetes object. At that point, I have to also provide my own documentation and tests, users face an additional learning hurdle, I have to worry about compatibility and when it's safe to remove, and the net result is a lot worse than if I had just stuck with the original line of code.

Again, I also openly admit that I don't stick to this rule consistently 😛

@floriankoch
Copy link

@anguslees ok, maybe you are right....

@jbianquetti-nami jbianquetti-nami deleted the jjo-fix-issue12-args_format branch October 13, 2020 09:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

args helper in the Container struct only supports args starting wit -- and and followed by =
4 participants