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

Remove the Ginkgo CLI #127

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Remove the Ginkgo CLI #127

merged 2 commits into from
Dec 9, 2022

Conversation

davewalter
Copy link
Member

@davewalter davewalter commented Jul 1, 2022

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:

  • introduces a new task
  • changes an existing task
  • changes the Dockerfile
  • introduces a breaking change (other users will need to make manual changes when this is released)

Did you update the README as appropriate for this change?

  • YES
  • N/A

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?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

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

@ctlong

@davewalter davewalter requested a review from ctlong July 1, 2022 20:37
@davewalter davewalter force-pushed the ginkgo-v2-cli branch 2 times, most recently from 2a7f07d to fdf91b7 Compare July 7, 2022 01:21
@sethboyles
Copy link
Member

sethboyles commented Jul 12, 2022

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
Copy link
Member Author

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
Copy link
Member

ctlong commented Jul 13, 2022

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

- Keep in sync with cf-acceptance-tests.
@davewalter
Copy link
Member Author

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
Copy link

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

@ctlong
Copy link
Member

ctlong commented Aug 31, 2022

@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
Copy link

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.

Ginkgo v2 requires the CLI version to match the local version in
the go.mod. Since we cannot predict the version used in individual
go.mod files we should not install the CLI here, and should instead look
to individual projects to vendor and use the tool appropriately.

See [here](https://github.com/onsi/ginkgo/releases/tag/v2.3.1) for the
release notes where Onsi outlines the versioning decision.
@davewalter davewalter changed the title Bump to v2 of the Ginkgo CLI Remove the Ginkgo CLI Nov 29, 2022
@ctlong
Copy link
Member

ctlong commented Dec 2, 2022

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.

@ctlong ctlong merged commit f3f59f2 into main Dec 9, 2022
@ctlong ctlong deleted the ginkgo-v2-cli branch December 9, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants