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

Ginkgo2 #543

Merged
merged 11 commits into from
Jun 25, 2022
Merged

Ginkgo2 #543

merged 11 commits into from
Jun 25, 2022

Conversation

moleske
Copy link
Member

@moleske moleske commented May 6, 2022

Are you submitting this PR against the develop branch?

All PR's to CATs should be submitted to develop and will be merged to main once they've passed acceptance.

What is this change about?

Describe the change and why it's needed.
bump to ginkgo2 to stay on supported versions

Please provide contextual information.

Include any links to other PRs, stories, slack discussions, etc... that will help establish context.
cloudfoundry/clock#31
cloudfoundry/cf-test-helpers#58
and a future pr to https://github.com/cloudfoundry/credhub-cli to get it to ginkgo2 (not actually necessary, just nice to have to remove all ginkgo v1 references)
are required before this can get further

What version of cf-deployment have you run this cf-acceptance-test change against?

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test?
  • changes an existing test
  • requires an update to a CATs integration-config

Did you update the README as appropriate for this change?

  • YES
  • N/A

If you are introducing a new acceptance test, what is your rationale for including it CATs rather than your own acceptance test suite?

CATs should validate common operator workflows.
CATs is not a regression test suite.
CATs is run by every component team to validate their releases before promotion.

How many more (or fewer) seconds of runtime will this change introduce to CATs?

🤷‍♀️

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!

It's helpful to tag a few other folks on your team or your team alias in case we need to follow up later.
just a random person bumping ginkgo2

@cf-rel-int-status-bot
Copy link

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! 👀

2 similar comments
@cf-rel-int-status-bot
Copy link

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! 👀

@cf-rel-int-status-bot
Copy link

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! 👀

@moleske moleske marked this pull request as ready for review June 3, 2022 23:25
@moleske
Copy link
Member Author

moleske commented Jun 3, 2022

Marking this ready not because I think it should be merged as is, but that it is in a state that could now have reasonable feedback. Tried running on a bosh lite, but I hit a timeout issue on test, so didn't finish the suite. There is some reporter stuff that I'd like some feed back on and I think I didn't put back colors (apparently I did put them back)

Comment on lines 130 to 157
//rs := []Reporter{}
//
//if validationError == nil {
// if Config.GetArtifactsDirectory() != "" {
// helpers.EnableCFTrace(Config, "CATS")
// rs = append(rs, helpers.NewJUnitReporter(Config, "CATS"))
// }
//}
//
//reporterConfig := Config.GetReporterConfig()
//
//if reporterConfig.HoneyCombDataset != "" && reporterConfig.HoneyCombWriteKey != "" {
// honeyCombClient := client.New(libhoney.Config{
// WriteKey: reporterConfig.HoneyCombWriteKey,
// Dataset: reporterConfig.HoneyCombDataset,
// })
//
// globalTags := map[string]interface{}{
// "run_id": os.Getenv("RUN_ID"),
// "env_api": Config.GetApiEndpoint(),
// }
//
// honeyCombReporter := honeycomb.New(honeyCombClient)
// honeyCombReporter.SetGlobalTags(globalTags)
// honeyCombReporter.SetCustomTags(reporterConfig.CustomTags)
//
// rs = append(rs, honeyCombReporter)
//}
Copy link
Member Author

Choose a reason for hiding this comment

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

The honeyCombReporter I think needs to be rewritten to be properly used. The junit reporter seems to be a flag on the command line now based on https://onsi.github.io/ginkgo/MIGRATING_TO_V2#improved-reporting-infrastructure. Curious on folks thoughts about this

Copy link
Member

Choose a reason for hiding this comment

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

I'd previously made an issue about this: #528. I'm in favor of removing it, but just in case I've started a conversation in slack.

- removed some color stuff cause not available in ginkgo2

Authored-by: Michael Oleske <moleske@pivotal.io>
ginkgo2

Authored-by: Michael Oleske <moleske@pivotal.io>
- still broken, need cf-test-helpers updated first
- might have future problem with honeycomb and junit reporters

Authored-by: Michael Oleske <moleske@pivotal.io>
Authored-by: Michael Oleske <moleske@pivotal.io>
Authored-by: Michael Oleske <moleske@pivotal.io>
- the done async style shouldn't be used, so replaced with eventually
  timeout (probably done correctly 🤷‍♀️)

Authored-by: Michael Oleske <moleske@pivotal.io>
- at least for now from bin/test, you need to specify a file location
  for it

Authored-by: Michael Oleske <moleske@pivotal.io>
Authored-by: Michael Oleske <moleske@pivotal.io>
Authored-by: Michael Oleske <moleske@pivotal.io>
@ctlong ctlong mentioned this pull request Jun 6, 2022
7 tasks
@moleske
Copy link
Member Author

moleske commented Jun 10, 2022

finally got around to running on a bosh lite. For what my bosh lite was configured to support, it was passing (120 passed, 0 failed, 3 pending, 105 skipped). So feeling pretty good about the ginkgo v2 minus the above about reporters

below is the configuration of what tests were included. I didn't have a windows cell set up to test anything there

  "include_apps": true,
  "include_capi_experimental": true,
  "include_detect": true,
  "include_security_groups": true,
  "include_services": true,
  "include_v3": true,
  "include_tasks": true,
  "include_backend_compatibility": false,
  "include_capi_no_bridge": false,
  "include_container_networking": false,
  "include_credhub" : false,
  "include_docker": false,
  "include_internet_dependent": false,
  "include_isolation_segments": false,
  "include_persistent_app": false,
  "include_private_docker_registry": false,
  "include_privileged_container_support": false,
  "include_route_services": false,
  "include_routing": false,
  "include_routing_isolation_segments": false,
  "include_service_discovery": false,
  "include_service_instance_sharing": false,
  "include_ssh": false,
  "include_sso": false,
  "include_zipkin": false

Authored-by: Michael Oleske <moleske@pivotal.io>
Copy link
Member

@ctlong ctlong left a comment

Choose a reason for hiding this comment

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

There is an ask to keep the Honeycomb reporter, but I don't think that should stop up this PR. I do think we should add the JUnit and cf trace stuff back in though as that's pretty easy to do.

Once that's done I think this is good to go, thanks @moleske !

assets/credhub-service-broker/go.mod Show resolved Hide resolved
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.

None yet

3 participants