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

Conformance Helpers Should be Cleaned Up #1728

Open
robscott opened this issue Feb 13, 2023 · 13 comments
Open

Conformance Helpers Should be Cleaned Up #1728

robscott opened this issue Feb 13, 2023 · 13 comments
Labels
area/conformance kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@robscott
Copy link
Member

What would you like to be added:
We currently have some duplicative methods that really should be unified before we add conformance tests for more types. For example, the following functions have significant overlap:

  1. GatewayAndHTTPRoutesMustBeAccepted + GatewayAndTLSRoutesMustBeAccepted

    func GatewayAndHTTPRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, controllerName string, gw GatewayRef, routeNNs ...types.NamespacedName) string {

    func GatewayAndTLSRoutesMustBeAccepted(t *testing.T, c client.Client, timeoutConfig config.TimeoutConfig, controllerName string, gw GatewayRef, routeNNs ...types.NamespacedName) (string, []v1beta1.Hostname) {

  2. HTTPRouteMustHaveParents + TLSRouteMustHaveParents

    func HTTPRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig config.TimeoutConfig, routeName types.NamespacedName, parents []v1beta1.RouteParentStatus, namespaceRequired bool) {

    func TLSRouteMustHaveParents(t *testing.T, client client.Client, timeoutConfig config.TimeoutConfig, routeName types.NamespacedName, parents []v1alpha2.RouteParentStatus, namespaceRequired bool) v1alpha2.TLSRoute {

Why this is needed:
Our conformance tests have grown organically over the years. Although each change made sense in isolation, these changes have resulted in a high likelihood that we'll start to have unexpected variations between tests for different route types.

Note: This is going to be complex to resolve, it would likely be good for someone that already has experience writing and/or running conformance tests to work on this. It may also be good to agree on a direction in this issue before investing too much time in writing code for this.

@robscott robscott added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 13, 2023
@robscott
Copy link
Member Author

/area conformance
/kind cleanup
/remove-kind feature

@k8s-ci-robot k8s-ci-robot added area/conformance kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Feb 13, 2023
@shaneutt shaneutt self-assigned this Feb 13, 2023
@shaneutt shaneutt added this to the v0.6.2 milestone Feb 13, 2023
@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 15, 2023
@shaneutt shaneutt removed their assignment Mar 8, 2023
@shaneutt shaneutt modified the milestones: v0.6.2, v0.7.0 Mar 14, 2023
@mmamczur
Copy link
Contributor

I looked at this code a bit yesterday.
I'm guessing it would be best if instead GatewayAndHTTPRoutesMustBeAccepted() one could use GatewayAndRoutesMustBeAccepted() and it works for all types of connected routes.
So the problem in general is that we have types that have similar structure (at least in the part relevant here), as these all have route.Status.Parents.
One options is that these could be read as unstructured (which doesn't look great). Another is to have some adapter per type implementing a function like GetStatus() . The problem is that you need to know what type you need to read upfront

route := &v1beta1.HTTPRoute{}
err := client.Get(ctx, routeName, route)

so you still don't know which adapter to use

@mmamczur
Copy link
Contributor

actually it is not possible to get a route without providing it's kind before. So you do need to know what type it is upfront.

@mmamczur
Copy link
Contributor

one way of doing this without writing code per route type is to use reflection to get RouteStatus, which all routes must have. Seems to work dfc708c

@robscott robscott modified the milestones: v0.7.0, v0.8.0 Mar 24, 2023
@youngnick
Copy link
Contributor

Neat. I've seen some similar stuff done before using the Unstructured type, and then deserializing it into a duck-type that only contains the bits you're interested in. This uses the json/yaml Go deserialization property that it drops unknown fields. (I've also been playing around with this for some tooling to read anything with status.Conditions :) )

@shaneutt shaneutt removed this from the v0.8.0 milestone May 18, 2023
@shaneutt shaneutt added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 18, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024
@youngnick
Copy link
Contributor

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 21, 2024
@youngnick
Copy link
Contributor

Seems like this would still be useful, so I'll unstale for now.

/remove-lifecycle rotten

@mlavacca @shaneutt for conformance cleanup task tracking.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 22, 2024
@mlavacca
Copy link
Member

mlavacca commented Jun 5, 2024

Yep, this is definitely still useful 👍

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 3, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle rotten
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

7 participants