cloudfoundry / cf-deployment-concourse-tasks

Apache License 2.0
23 stars 76 forks source link

Remove the Ginkgo CLI #127

Closed davewalter closed 1 year ago

davewalter commented 2 years ago

What is this change about?

~This PR bumps the version of the Ginkgo CLI included in the docker image to v2 to match the version used in CATs.~

Edit: This PR removes the ginkgo CLI from the docker image, as the version of the CLI must match the local Ginkgo library version in any given project. Therefore, we need to push the responsibility onto project owners to vendor and use the tool appropriately.

Please provide contextual information.

Please check all that apply for this PR:

Did you update the README as appropriate for this change?

How should this change be described in release notes?

~This PR bumps the version of the Ginkgo CLI included in the docker image to v2 to match the version used in CATs.~ This PR removes the ginkgo CLI from the docker image.

What is the level of urgency for publishing this change?

Tag your pair, your PM, and/or team!

@ctlong

sethboyles commented 2 years ago

In the CAPI pipeline, which was pointing at CATS release-candidate, some of our CATS runs were failing with Ginkgo 2 because the default timeout in Ginkgo 2 was changed to 1 hour (from 24 hours):

Finally, the default timeout has been reduced from 24h down to 1h. Users with long-running tests may need to adjust the timeout in their CI scripts

https://onsi.github.io/ginkgo/MIGRATING_TO_V2#timeout-behavior

It would be great if this could PR could also introduce a timeout options that is set to something higher to 1 hour--maybe 2 or 3?

davewalter commented 2 years ago

Thanks for the feedback @sethboyles. I've updated the run-cats task to introduce a new TIMEOUT parameter as part of this change. The default for this value is 2 hours, which should be enough time for CATs to run.

I also introduced the Ginkgo --no-color flag as I have noticed that Concourse v7 renders the red text output as flashing text, which makes it very hard to read.

ctlong commented 2 years ago

Rebased and force-pushed after merging #128 to fix merge conflicts.

davewalter commented 2 years ago

I've pulled the changes to the run-cats task out into a separate PR (#129) as I realised that the CATs bin/test script ignores the version of the Ginkgo CLI in the image and installs the version that is vendored into CATs instead. This means that anyone using this task is already using the v2 CLI, and is seeing warnings like this at the end of the run:

You're using deprecated Ginkgo functionality:

--keepGoing is deprecated, use --keep-going instead Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#changed-command-line-flags --noisySkippings is deprecated Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed--noisypendings-and--noisyskippings --randomizeAllSpecs is deprecated, use --randomize-all instead Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#changed-command-line-flags --skipPackage is deprecated, use --skip-package instead Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#changed-command-line-flags --slowSpecThreshold is deprecated use --slow-spec-threshold instead and pass in a duration string (e.g. '5s', not '5.0') Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#changed--slowspecthreshold --flakeAttempts is deprecated, use --flake-attempts instead Learn more at: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#changed-command-line-flags

PR #129 should address these warnings without needing to bump the version of the Ginkgo CLI included in the image. We should wait to merge this until we have cut a new release of CATs with the Ginkgo v2 changes.

samcolson4 commented 2 years ago

@davewalter - CATs 8.0.0 appears to satisfy your requirement note, unless I have misread this thread. Can this PR now be merged?

ctlong commented 2 years ago

@samcolson4 I think we're ready to merge this PR but have held off so far out of concern for the impact on folks who have not upgraded their projects to use ginkgo v2 and pull in this docker image with the latest tag.

We'd planned to send a message to the cf-dev mailing list first, to raise awareness of the change. However, other issues have come up in the last weeks that have diverted our attention.

samcolson4 commented 2 years ago

Just to note, when this PR gets merged, the lines I added in this PR can be reverted back to the old style of calling Ginkgo.

ctlong commented 1 year ago

I've just sent out the cf-dev announcement. I figure we can wait a week or so to see if folks have thoughts / comments, and then look to move forward with this PR.