search

argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
15.11k stars 3.21k forks source link

refactor(deps)!: remove `swagger-ui-react` #13818

Closed agilgur5 closed 3 weeks ago

agilgur5 commented 3 weeks ago

Partial fix for #13410 -- see #13821 for the other half to downgrade react that is a pure fix (vs this is a breaking change that is sizeable in its own right) Replaces #13069 to resolve CVEs by just removing swagger-ui-react entirely

Motivation

Removes the Swagger UI entirely from the UI as it is a gigantic dep (2nd largest only behind Monaco Editor) and often has vulnerable dependencies that are difficult to resolve without affecting other deps. According to #12061, this would shave off ~2MB / 826KB minified / 250KB gzipped from the UI bundle and will also speed up builds and installs as such.

It also no longer necessary to have in the UI as there is a Swagger UI in the docs after https://github.com/argoproj/argo-workflows/pull/10923 and the docs are versioned after https://github.com/argoproj/argo-workflows/issues/11390

Modifications

Docs update

update and simplify REST API page

Verification

Manually tested and confirmed the page works as intended. Screenshot of new API docs page: Screenshot 2024-10-25 at 4 03 36 PM

Future Work

  1. [x] Downgrade React to fix a bunch of compat warnings (and unexpected errors like https://github.com/argoproj/argo-workflows/pull/13593) etc per above linked #13410, https://github.com/argoproj/argo-workflows/pull/13069#issuecomment-2159579279, etc
    • EDIT: Completed in #13821
  2. [x] Have the versioned API reference page pull its reference from the same version GH, instead of main
    • EDIT: Completed in #13824
tooptoop4 commented 3 weeks ago

if swagger ui is removed from each operator's server should there be an entry in breaking changes/upgrading guide?

agilgur5 commented 3 weeks ago

It is breaking, so we could, although the UI itself tells you what to do and there is no upgrade instruction otherwise. The impacted UI page having a message for a deprecation window for the Archived List UI could have made sense too, i.e. a better option than https://github.com/argoproj/argo-workflows/pull/13371

I'm inclined to agree and note it in this case, although we do want to be careful about making the UI a "breakable" surface area and where to draw boundaries as it's not quite the same as the schema / API / etc "Public API" (also I noticed metrics are missing there) but rather a consumer of those.

Joibel commented 3 weeks ago

Could this be added to docs/upgrading.md to note the removal of the feature?

agilgur5 commented 3 weeks ago

Added an upgrading note in https://github.com/argoproj/argo-workflows/pull/13818/commits/d7062396bf6536da49558aea0ed689dd0ab7673c

Joibel commented 3 weeks ago

@agilgur5, could you resolve the conflicts and then we can merge and cut a 3.6.0-rc4 with this in?

MasonM commented 3 weeks ago

@Joibel I can take this over if you'd like, though I don't have access to push to https://github.com/agilgur5/argo-workflows/tree/refactor-remove-swagger-ui (which I'm assuming I'll get when my membership request goes through).

I'm assuming the best way forward is to create a new PR with a branch forked from https://github.com/agilgur5/argo-workflows/tree/refactor-remove-swagger-ui. Does that sound reasonable?

agilgur5 commented 3 weeks ago

It requires write access on GH to push to PR, or Approver in Argoproj terms.

I just added you as a collaborator to my fork for simplicity

I'm assuming the best way forward is to create a new PR with a branch forked from

Otherwise that would be the general process that has been done in the past and tends to be an easy way to give proper attribution

MasonM commented 3 weeks ago

@agilgur5 Thanks! It works, and I resolved the conflicts