fix(api): deleteDelayDuration should be a string

[agilgur5]

Fixes #12631 , which is due to a tooling bug that I noticed in one of my first ever PRs to Argo: #11325 (comment)

Motivation

• the CRD tooling gets this right because the k8s apimachinery package has code that kube-openapi generator understands: https://github.com/kubernetes/apimachinery/blob/2465dc5239ab8827a637148a78b380c278b4a5f4/pkg/apis/meta/v1/duration.go#L65
• meanwhile, the API tooling is different and not k8s-specific, so it doesn't know how to properly handle this type
  • this results in the API spec showing an incorrect schema, the one of the unmarshaled struct, instead of a plain string

Modifications

• workaround the tooling issues by declaring it as a string and parsing the string to a time.Duration ourselves

  • i.e. instead of the k8s metav1.Duration struct's unmarshaling

• re-use ParseStringToDuration util func

  • rename its internal variables to not be specific to suspend as it's used by other fields as well
  • remove the duplicate func in operator.go

• add warning if an error occurs during parsing

Verification

Notes to Reviewers

This fix is backward-compatible as it does not affect the CRDs (which were correct already) and only fixes the broken API piece.

Future Work

1. This should be changed to deleteDelaySeconds per k8s convention as mentioned in deleteDelayDuration JSON schema and validation mismatch #12631 (comment). That would also obviate the need for this entirely as well. Changing the other durations in the spec to seconds would also remove the need for duration parsing code as well

[agilgur5]

Codegen is failing and seemingly resulting in the rest of the failures.

The diff in CI makes sense, but I oddly can't seem to get my local env to reproduce it (which sometimes errors out during codegen of this particular change). might be time to rebuild the devcontainer from scratch 🤷

[agilgur5]

might be time to rebuild the devcontainer from scratch 🤷

yea that fixed it. and sped up my devcontainer a bit for whatever reason

[terrytangyuan]

FYI you can make the PR as draft when you create the PR.

[agilgur5]

I know, my codegen was wacky locally (it did a partial generation somehow), so I didn't know it was incorrect until CI showed that, which is when I changed it to a draft

[juliev0]

Approved. Out of curiosity, what are all the ways that we're using the json schema? Is it just being used in the Swagger documentation or more than that?

[agilgur5]

The SDKs are generated off of it as well (and apparently the field docs too??), as the generated changes here show.

It's also used for IDE schema support and the schema support in the UI editor. And people use it with other generators and tooling too, like Jsonnet in the Slack thread I linked in the issue, and like cdk8s in #13530

So it primarily affects tooling. Which can be quite confusing as the schema doesn't match the actual API, so you'll get false positive schema errors as a result.
And the schema has some issues because of the tooling (non-k8s-aware) and steps used to generate it (Go -> protobuf -> OpenAPI, vs the k8s tooling is Go -> OpenAPI directly). Tooling on tooling

[philBrown]

This seems like a breaking change released as a patch version. Not very good semver

[Joibel]

I agree. I did the release of 3.5.11 but this shouldn't have been in it. It should have been marked with ! in the title to prevent it being backported as it is breaking.

@philBrown we don't follow semver but this should have waited for 3.6.

@agilgur5, @juliev0, @terrytangyuan - I'm happy to revert and release a 3.5.12 undoing this. Opinions?

[philBrown]

FYI I had to update our code with something like

if duration, err := wf.Spec.PodGC.GetDeleteDelayDuration(); err == nil {
    //...
}

Thanks for the utility function

[agilgur5]

Per the PR description, it's not breaking:

Notes to Reviewers

This fix is backward-compatible as it does not affect the CRDs (which were correct already) and only fixes the broken API piece.

The types did not change as it was always a string, except in the API, where it did not work.

[philBrown]

it's not breaking

@agilgur5 I beg to differ...

• I have consumer code using the Golang module
• I updated the patch release
• My code is now broken

[agilgur5]

using the Golang module

Internal Go types are internal. Golang doesn't have a way of defining "published" or "not published" given you can use any Git repo as a source. There are no boundaries defined there: if the entire module were considered "public", then many many internal changes and refactors would be considered breaking.

The Public API is defined by schemas, such as the CRD and OpenAPI schema.
There is a Golang SDK which connects to the API and therefore follows its schema.

[agilgur5]

Thanks for the utility function

For reference, that is an internal helper as well (it is used in the PR). As a relevant example, if we were to remove that, that should not be a breaking change, since it's internal.
However, if you are using it, that would indeed break your usage. We unfortunately don't really have a way of regulating that to disallow usage. That being said, there is also no user-facing documentation on using it either -- as it is not intended for external usage -- unlike the schemas and SDKs.

Using the types from the schema is a stable surface that can be relied on instead. And SDKs can be/are versioned separately. But the internal types are not versioned since they are internal. It would be a pretty big effort to make them able to be versioned too.

[Joibel]

using the Golang module

Internal Go types are internal. Golang doesn't have a way of defining "published" or "not published" given you can use any Git repo as a source. There are no boundaries defined there: if the entire module were considered "public", then many many internal changes and refactors would be considered breaking.

For future reference, it is possible to make un-importable modules in go: https://pkg.go.dev/cmd/go#hdr-Internal_Directories

Argo Workflows hasn't done this, for better or for worse.