v3.5.9 UI: Clicking pod logs button crashes UI

[tico24]

Pre-requisites

• I have double-checked my configuration
• I have tested with the :latest image tag (i.e. quay.io/argoproj/workflow-controller:latest) and can confirm the issue still exists on :latest. If not, I have explained why, in detail, in my description below.
• I have searched existing issues and could not find a match for this bug
• I'd like to contribute the fix myself (see contributing guide)

What happened? What did you expect to happen?

• Install 3.5.9
• Run a workflow
• Click a node on the running workflow
• Click 'logs' in the 'summary' tab
  crash:

There are no relevant server logs.

Workaround:
Roll back to 3.5.8

Edit: I would encourage others to use the thumb emoji rather than an unnecessary "we have this issue too"-style comment.

Version(s)

v3.5.9

Paste a minimal workflow that reproduces the issue. We must be able to run the workflow; don't enter a workflows that uses private images.

Any workflow.

Logs from the workflow controller

Nothing useful

Logs from in your workflow's wait container

not relevant

[ab-sk-no]

Verified, we have that, too.

[tchatzig]

Just upgraded from v3.5.8 to v3.5.9 and we have the same issue.

[agilgur5]

This appears to be due to out-of-order cherry-picking; #12973 was cherry-picked before #12964. In fact, it was cherry-picked into an older patch (v3.5.7) because it was a fix, while #12964 is a refactor that was cherry-picked only for 3.5.9 in order to resolve merge conflicts for #13039 etc.

I'm pretty surprised this didn't get caught by static analysis by the type-checker during build or the linter. The release-3.5 branch never brought in linter migrations (#12163), but this would still be surprising that an older linter wouldn't detect it 🤔 My type-checker in my editor detects this instantly.
EDIT1: Can confirm that yarn lint on release-3.5 does not detect this (even if I fix all the stylistic issues it points out due to linter differences from main). Type-check on release-3.5 fails on some node_modules though (even with skipLibCheck, apparently due to ramda/types#64 in a transitive dep). I'm still not sure how the build's ts-loader doesn't catch this on type-check though. That's odd 🤔
EDIT2: Apparently pure ESLint doesn't detect this either when I make the same problem on main. But on main, yarn lint also runs the type-checker after #12516, which does catch it and so would catch it in CI.

[agilgur5]

I'm still not sure how the build's ts-loader doesn't catch this on type-check though. That's odd 🤔

I suspected that the transpileOnly flag doesn't actually accept false and is just "supplied or not". Removing the flag indeed causes type errors to pop up: this one, but also a few other internal ones (that are purely type-related and do not affect the resulting JS), and the node_modules ones due to ramda/types#64. As a result, I can't quite enable that in the release-3.5 branch to prevent this in the future. I think it might be worthwhile in this case to backport UI build/toolchain changes from main, but will confer with other contributors as it is outside our normal process.

In the interim, I'll push up a hotfix for this and should have 3.5.10 out tonight as this is affecting lots of people; fastest rate of upvotes I've seen in this repo (I guess it's good the UI is popular at least 😅 )

[agilgur5]

This has been fixed in 72d0d22 and included in v3.5.10

(commit ref is bc it only exists on the release-3.5 branch since this bug does not exist on main as it was due to out of order cherry-picking per above)

[tchatzig]

Thanks @agilgur5
that was a quick fix 💯

[ab-sk-no]

Indeed. Very quick. Thank you!

[tataue]

Thank you!