[SDK] Fix withItem loop

[numerology]

Partially fix #2570

Question: I can imagine for withparams we have the same problem if user provides arbitrary names in the loop variable's runtime value. How to prevent that from happening? Shall we bypass k8s name sanitization in this case?

---

This change is 

[Ark-kun]

we have the same problem if user provides arbitrary names in the loop variable's runtime value

There might be a problem if the user has non-Kubernetes keys in their dictionaries since the {{item.*}} placeholders might be converted. The runtime values won't be a problem. But the placeholder might not be able to access them (e.g. you cannot access keys with spaces).

[Ark-kun]

I'm not sure this line does anything.

[numerology]

we have the same problem if user provides arbitrary names in the loop variable's runtime value

There might be a problem if the user has non-Kubernetes keys in their dictionaries since the {{item.*}} placeholders might be converted. The runtime values won't be a problem. But the placeholder might not be able to access them (e.g. you cannot access keys with spaces).

That's true. So shall we alert the user whenever they want to access a non-Kubernetes keys in their ContainerOp, for example, try to use item.a_a, which will be converted to a-a under the hood?

[Ark-kun]

Nit: for argument_set in loop_tasks:

[Ark-kun]

/lgtm

[Ark-kun]

So shall we alert the user whenever they want to access a non-Kubernetes keys in their ContainerOp, for example, try to use item.a_a, which will be converted to a-a under the hood?

That would be great. But there is a problem - python requires underscores while kubernetes requires dashes. So the names will be converted often.

Update. I just remember that this is not Kubernetes limitation, but rather Argo limitation. And Argo lifted it in 2.3.0. So maybe we should just stop converting underscores to dashes. Then the problem will mostly go away.

[Ark-kun]

/lgtm

[numerology]

So shall we alert the user whenever they want to access a non-Kubernetes keys in their ContainerOp, for example, try to use item.a_a, which will be converted to a-a under the hood?

That would be great. But there is a problem - python requires underscores while kubernetes requires dashes. So the names will be converted often.

Update. I just remember that this is not Kubernetes limitation, but rather Argo limitation. And Argo lifted it in 2.3.0. So maybe we should just stop converting underscores to dashes. Then the problem will mostly go away.

Thanks for the info! If that's the case, I'd prefer the latter way, because as discussed in the related issue, current fix cannot fix the problem with withParams. I think it would be better off if we just do not use sanitized name in this case.

Let me try to improve it a little bit and will let you know when I'm done.

[Ark-kun]

Let me try to improve it a little bit and will let you know when I'm done.

It's fine to do that in another PR though. It might require some sizable amount of work and testing.

[numerology]

Let me try to improve it a little bit and will let you know when I'm done.

It's fine to do that in another PR though. It might require some sizable amount of work and testing.

Okay.

[Ark-kun]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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:

• sdk/OWNERS [Ark-kun]

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