Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(docs): replace outdated whalesay image with busybox #13429

Merged
merged 3 commits into from
Aug 3, 2024

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Aug 2, 2024

Fixes #11858, Fixes #13388, Fixes #12767 (comment)

Motivation

The whalesay image was last updated 9 years ago and does not support Docker Manifest v2 nor arm64 architectures.
Per docker/whalesay#6, where I tweeted at Docker and emailed support twice, Docker has no intention to update the image (it just requires a rebuild, that's it... 🤷 ) nor even mark it as deprecated or archived... 😕

Modifications

  • replace whalesay image with busybox image
    • also instead of cowsay, use plain echo
    • allow busybox in tested examples

secondary modifications

  • rename several templates that were named whalesay unnecessarily as well

    • they often just printed a message, and so are now named print-message
    • others always printed "hello world", and so are now named hello-world
    • some didn't even use the whalesay image, and so are now named more accordingly
  • also clarify a few other template names that would do more than print a message

    • now named print-message-from-file or hello-world-to-file
  • plus make some small style adjustments per the docs style guide while at it

    • and remove some trailing newlines in codeblocks

Verification

  • No more results for whalesay or cowsay in docs and examples
    • well, there are a few leftover, from the upgrading guide and the configmap, where whalesay is very intentionally used as a manifest v1 example. also a few cowsay ones leftover are all from the argosay image, see "Future Work" below
  • Some of these, like the "hello world" doc, I manually tested
  • The examples have an E2E suite that tests them

Future Work

  1. Replace argosay as well and remove the argosay build entirely from the codebase
  2. Replace both argosay and whalesay in all tests
    • there seems to be 1000+ references in the tests 😕
    • they don't seem to cause problems yet and aren't user-facing

- also instead of `cowsay`, use `echo`

- rename several templates that were named `whalesay` unnecessarily as well
  - they often just printed a message, and so are now named `print-message`
  - some didn't even use the `whalesay` image and so seemed erroneously named
  - others always printed "hello world", and so are now named `hello-world`

- also clarify a few other template names that would do more than print a message
  - now named `print-message-from-file` or `hello-world-to-file`

- plus make some small style adjustments per the style guide while at it
  - and remove some trailing newlines in codeblocks

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added area/docs Incorrect, missing, or mistakes in docs prioritized-review For members of the Sustainability Effort labels Aug 2, 2024
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Aug 2, 2024
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@juliev0
Copy link
Contributor

juliev0 commented Aug 2, 2024

Thanks for doing this.

The examples have an E2E suite that tests all of them

Oh, nice. I was going to ask about that.

@agilgur5 agilgur5 merged commit 902ddd4 into argoproj:main Aug 3, 2024
19 checks passed
@agilgur5 agilgur5 deleted the fix-docs-replace-whalesay branch August 3, 2024 02:11
@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 4, 2024

  • The examples have an E2E suite that tests them

It actually didn't run here apparently since no source or test code changed 😕
I did run it myself though (hence the last commit), and it ran on later PRs.

Changing the diff detection is actually a little easier said than done though, as it wraps the whole E2E matrix, and this is only one of the test suites. The more correct way to do it would probably be to split off the examples from the matrix as its own job that runs if the e2es should run or if the examples changed. But that may require a pretty decent-sized refactor.

  • Some of these, like the "hello world" doc, I manually tested

I've also been meaning to replace many of the docs examples with embeds to the examples/ dir. That would reduce duplication, help with single source-of-truth, aid discovery, and make them easily testable (if they are not existing duplicates), etc.

elliotgunton added a commit to argoproj-labs/hera that referenced this pull request Aug 19, 2024
See argoproj/argo-workflows#13429

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
elliotgunton added a commit to argoproj-labs/hera that referenced this pull request Aug 19, 2024
See argoproj/argo-workflows#13429

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
elliotgunton added a commit to argoproj-labs/hera that referenced this pull request Aug 19, 2024
See argoproj/argo-workflows#13429

Fixes Python for upstream files so `make regenerate-test-data` works as
expected (to unblock #1157)

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Joibel pushed a commit to pipekit/argo-workflows that referenced this pull request Sep 19, 2024
…#13429)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Joibel pushed a commit that referenced this pull request Sep 20, 2024
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs prioritized-review For members of the Sustainability Effort
Projects
None yet
2 participants