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

Get to ginkgo v2 #2378

Closed
wants to merge 15 commits into from
Closed

Get to ginkgo v2 #2378

wants to merge 15 commits into from

Conversation

moleske
Copy link
Member

@moleske moleske commented Feb 25, 2023

Update - panics all gone, one failing test as described in this comment (below here) that I need help resolving. Everything else seems to be working. I think future work would need to give thought if it is worth trying to backport this at all to v8 and/or v7

old original description details

  • panic occuring because of mismatch ginkgo versions due to ginkgo v1 being needed by code.cloudfoundry.org/cfnetworking-cli-api
  • code.cloudfoundry.org/cfnetworking-cli-api is archived and hasn't been maintained for quite awhile
  • the error shows as /Users/moleske/workspace/cli/util/clissh/clissh.test flag redefined: ginkgo.seed panic: /Users/moleske/workspace/cli/util/clissh/clissh.test flag redefined: ginkgo.seed

goroutine 1 [running]:
flag.(*FlagSet).Var(0x14000116120, {0x1049bcdf8, 0x104c3e280}, {0x1400011d020, 0xb}, {0x104842d23, 0x2a})
/opt/homebrew/Cellar/go/1.20.1/libexec/src/flag/flag.go:982 +0x2a4
flag.(*FlagSet).Int64Var(...)
/opt/homebrew/Cellar/go/1.20.1/libexec/src/flag/flag.go:769
github.com/onsi/ginkgo/config.Flags(0x1049370c0?, {0x10482abdb?, 0x104843acb?}, 0x1)
/Users/moleske/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/config/config.go:75 +0xe4
github.com/onsi/ginkgo.init.0()
/Users/moleske/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/ginkgo_dsl.go:53 +0x34

Ginkgo ran 20 suites in 10.659707541s
Test Suite Failed
make: *** [units-non-plugin] Error 1

cc @cloudfoundry/wg-app-runtime-interfaces-cli-approvers if you all have thoughts
Thank you for contributing to the CF CLI! Please read the following:

  • Please make sure you have implemented changes in line with the contributing guidelines
  • We're not allowed to accept any PRs without a signed CLA, no matter how small.
    If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
  • All new code requires tests to protect against regressions.
  • Contributions must be made against the appropriate branch. See the contributing guidelines
  • Contributions must conform to our style guide. Please reach out to us if you have questions.

Does this PR modify CLI v6, CLI v7, or CLI v8?

nah
Please see the contribution doc above or review Architecture Guide.

Description of the Change

get to ginkgo v2 hotness

We must be able to understand the design of your change from this description.
Keep in mind that the maintainer reviewing this PR may not be familiar with or
have worked with the code here recently, so please walk us through the concepts.

Why Is This PR Valuable?

What benefits will be realized by the code change? What users would want this change? What user need is this change addressing?
cause ginkgo v1 not supported no more

Why Should This Be In Core?

Explain why this functionality should be in the cf CLI, as opposed to a plugin.

Applicable Issues

List any applicable GitHub Issues here

How Urgent Is The Change?

Is the change urgent? If so, explain why it is time-sensitive.

Other Relevant Parties

Who else is affected by the change?

@moleske
Copy link
Member Author

moleske commented Feb 25, 2023

if you all have thoughts on proper way to remove code.cloudfoundry.org/cfnetworking-cli-api as a dependency give a holler

@a-b
Copy link
Member

a-b commented Aug 15, 2023

@moleske this is a great initiative - V2 brings new tools to navigate flakes. @ccjaimes and @pivotalgeorge might be interested in this too

@moleske
Copy link
Member Author

moleske commented Sep 15, 2023

reading the commit history of the archive repo that we depend on (https://github.com/cloudfoundry-incubator/cfnetworking-cli-api/commits/master) that is causing problems here - it seems like that repo was created by ripping it out of the cli code initially. Seems reasonable to me to just bring it back if we're not maintaining it in that archived repo no longer

@moleske
Copy link
Member Author

moleske commented Sep 15, 2023

apparently same thing happened with https://github.com/cloudfoundry/gofileutils/commits/master

Copy link
Member Author

@moleske moleske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very specifically would like feedback on 80e4086 and 9e7aaec

I unilaterally made a decision in this pr to copy the code from those archived repos and move them here. Since they are archived they no longer get updates. They also weren't getting updates for years. It seems they were one extracted long ago for reasons, but not clear to me why they were extracted long ago. I'd like feedback if this seems like a correct move since that code was stale in those repos and they used to be here

I didn't actually need to change these cause I misunderstood where the ginkgo seed error was occurring. I've reverted them but that doesn't excuse the fact they aren't maintained

api/cfnetworking/networking_connection.go Outdated Show resolved Hide resolved
@moleske moleske changed the title First pass at getting to ginkgo v2 Get to ginkgo v2 Sep 18, 2023
@moleske moleske marked this pull request as ready for review September 18, 2023 23:06
@a-b
Copy link
Member

a-b commented Feb 1, 2024

We have a bunch of green units by disabling parallel execution.

Copy link

linux-foundation-easycla bot commented Feb 5, 2024

CLA Not Signed

@moleske
Copy link
Member Author

moleske commented Feb 5, 2024

/easycla

@moleske
Copy link
Member Author

moleske commented Feb 5, 2024

integration tests are failing cause they are using ginkgo v1, as that's what is currently in the main branch. this branch/pr updates the integration tests to use ginkgo v2. which will cause problems with v8/v7 branches when this is merged. This may be acceptable if we immediately work on porting those branches after this pr is merged

this pr won't go green until the main branch is updated to use ginkgo v2 in the integration tests

- panic occuring because of mismatch ginkgo versions due to ginkgo v1 being needed by code.cloudfoundry.org/cfnetworking-cli-api
- code.cloudfoundry.org/cfnetworking-cli-api is archived and hasn't been maintained for quite awhile
- the error shows as
/Users/moleske/workspace/cli/util/clissh/clissh.test flag redefined: ginkgo.seed
panic: /Users/moleske/workspace/cli/util/clissh/clissh.test flag redefined: ginkgo.seed

goroutine 1 [running]:
flag.(*FlagSet).Var(0x14000116120, {0x1049bcdf8, 0x104c3e280}, {0x1400011d020, 0xb}, {0x104842d23, 0x2a})
	/opt/homebrew/Cellar/go/1.20.1/libexec/src/flag/flag.go:982 +0x2a4
flag.(*FlagSet).Int64Var(...)
	/opt/homebrew/Cellar/go/1.20.1/libexec/src/flag/flag.go:769
github.com/onsi/ginkgo/config.Flags(0x1049370c0?, {0x10482abdb?, 0x104843acb?}, 0x1)
	/Users/moleske/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/config/config.go:75 +0xe4
github.com/onsi/ginkgo.init.0()
	/Users/moleske/go/pkg/mod/github.com/onsi/ginkgo@v1.16.4/ginkgo_dsl.go:53 +0x34

Ginkgo ran 20 suites in 10.659707541s
Test Suite Failed
make: *** [units-non-plugin] Error 1
- still have the seed redefined problem in clissh tests
…ack to cli

- it's been deprecated/archived so not getting updates
- and it was using ginkgo v1
- bumped to ginkgo v2
- attempted to fix networking_connection.go to use errors.As since that's a go 1.20 thing we fixed in 325af3c
- but it only fixed one test and I don't understand why the other is failing
- still seed flag errors
- used to be here many years ago
- is now archived so maintaining it here makes more sense
- removed the ginkgo v1 out of the couple of tests
- final ginkgo v1 thing (no longer appears in go.mod)
- Newer lager was needed with newer diego-ssh
moleske and others added 9 commits February 5, 2024 15:41
- don't need to acknowledge since this moves to ginkgo v2
… and using non deprecated pty

Since the issue seems related to running in parallel, we'll force the ssh_test to run in serial
Co-authored-by: Cristhian Peña <ccjaimes@users.noreply.github.com>
@a-b
Copy link
Member

a-b commented Mar 22, 2024

Delivered via #2810

@a-b a-b closed this Mar 22, 2024
@moleske moleske deleted the get-to-ginkgov2 branch March 22, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants