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

improvement(dev-ex): compatible/identical types should be aliased and reused where possible #38

Open
the-wondersmith opened this issue May 14, 2024 · 12 comments
Labels
enhancement New feature or request

Comments

@the-wondersmith
Copy link
Contributor

the-wondersmith commented May 14, 2024

It would dramatically improve the crate's usability if instead of defining discrete types for every nested object within a CRD, compatible/identical types were defined once and automatically reused.

For example, the HeaderMatch type is identical between GRPCRoute and HTTPRoute resources, and do not require discrete handling when writing code to handle them. Another good example would be the way that HTTPRouteFilters are a superset of GRPCRouteFilters and can therefore also be handled by the same code without special consideration (i.e. any code that handles all HTTPRouteFilter types inherently also handles all GRPCRouteFilter types).

As the crate currently is, a dev would have to write such code with the newtype pattern or work a bit of black magic to skirt the orphan rule. A much more ergonomic solution would be if types that were actually the same to simply have exported aliases where you'd expect them to be ([example]) or for types where the compatible type is effectively a subset to impl From<SubsetType> for SupersetType ([example]).

In the case of aliased types, I'd imagine we'd want something like:

//! NOTE - module structure and naming are for
//! illustrative purposes only, actual real world
//! names and structure should be assumed to carry
//! on as they currently are

pub mod shared {
    // ...

    #[derive(
        Eq,
        Clone,
        // ...
    )]
    #[serde(tag = "type", rename_all = "PascalCase")]
    pub enum HeaderMatch {
        // ...
    }

   // ...
}

pub mod grpcroute {
    // ...

    pub type GRPCHeaderMatch = super::shared::HeaderMatch;

    // ...
}

pub mod httproute {
    // ...

    pub type HTTPHeaderMatch = super::shared::HeaderMatch;

    // ...
}

and in the case of compatible types:

//! NOTE - see note in example above re: naming and structure

pub mod shared {
    // ...

    #[derive(
        Eq,
        Clone,
        // ...
    )]
    #[serde(tag = "type", rename_all = "PascalCase")]
    pub struct RequestHeaderFilter {
        // ...
    }

    #[derive(
        Eq,
        Clone,
        // ...
    )]
    #[serde(tag = "type", rename_all = "PascalCase")]
    pub struct RequestMirrorFilter {
        // ...
    }

   // ...
}

pub mod grpcroute {
    // ...

    #[serde(tag = "type", rename_all = "PascalCase")]
    pub enum GrpcRouteFilter {
        // ...

        /// ...
        #[serde(rename_all = "camelCase")]
        RequestHeaderModifier {
            request_header_modifier: super::shared::RequestHeaderFilter,
        }

        // ...
    }

    impl From<GrpcRouteFilter> for super::httproute::HttpRouteFilter {
        // ...
    }
}

pub mod httproute {
    // ...

    #[serde(tag = "type", rename_all = "PascalCase")]
    pub enum HttpRouteFilter {
        // ...
    
        #[serde(rename_all = "camelCase")]
        RequestMirror {
            request_mirror: super::shared::RequestMirrorFilter,
        },

        /// ...
        #[serde(rename_all = "camelCase")]
        RequestHeaderModifier {
            request_header_modifier: super::shared::RequestHeaderFilter,
        }

       // ...
    }
}
@shaneutt shaneutt added enhancement New feature or request labels May 14, 2024
@shaneutt
Copy link
Member

@the-wondersmith @kflynn and myself all sync'd about this recently. This would be a large boon to the ergonomics of using the library. There were a few options on the table, but we all agreed doing this at the CRD level in the upstream repository might be the best one because it has the additional potential to help other languages and clients in the future.

/cc @aryan9600 as this also relates to our statuses work in #37

@the-wondersmith
Copy link
Contributor Author

@shaneutt @kflynn just as a general "keep alive", what are our next steps here?

@shaneutt
Copy link
Member

shaneutt commented May 21, 2024

(Don't worry, this has not fallen off my radar I have a reminder set for it)

I was hoping to hear from @aryan9600 (ping!) but I think the general idea of the Gateway API project providing CRD information for client generation seems generally helpful and reasonable to ask for at some level. Basically any proposal should follow the GEP process, and in this case I think it would be fair to start a discussion referencing both this issue and #39 and reasoning, and then we can continue discussion there as well as bring it up at the community meeting.

Once it's had some time for discussion, we'll see if we can gather any other allies and then ultimately find out who are going to be the champion(s) driving it forward to completion. Please feel free to start that discussion!

oh and @astoycos and @clux, you might find this interesting as well.

@shaneutt shaneutt added this to the Initial Pre-Release milestone May 23, 2024
@shaneutt
Copy link
Member

@the-wondersmith checking in?

@the-wondersmith
Copy link
Contributor Author

@shaneutt apologies, I genuinely thought I'd opened that discussion. I've for sure done so now 😅.

Excited to get some feedback there

@shaneutt
Copy link
Member

No worries! Let's discuss there for a bit and see what people think 🫡

@shaneutt
Copy link
Member

shaneutt commented Aug 5, 2024

I think we should try to bring this topic up in a Gateway API community meeting to try to spark more interest. If no further interest can be sparked, then it might be a situation where we just ask (the Gateway API community) "is anyone opposed to us creating a GEP for this?" and see where we can go from there.

@the-wondersmith
Copy link
Contributor Author

I think we should try to bring this topic up in a Gateway API community meeting {...}

Agreed. When / where is that?

@shaneutt
Copy link
Member

shaneutt commented Aug 5, 2024

https://gateway-api.sigs.k8s.io/contributing/#meetings <- it alternates, but right now: 3 weeks it's Monday at 3pm PST and one week it's Tuesday at 8am PST.

@kflynn
Copy link
Contributor

kflynn commented Aug 5, 2024

(Don't we now alternate 3PM PST and 8AM PST every other week?)

@shaneutt
Copy link
Member

shaneutt commented Aug 5, 2024

Ooop yep, Flynn is right 👍

@shaneutt
Copy link
Member

shaneutt commented Sep 9, 2024

So we haven't gotten any feedback in upstream, so I guess it's really up to us how motivated we are, and what specific way we're motivated to do this. My thoughts at this point is that this would be ultimately more beneficial to add in kopium because I expect it will have a positive impact on many projects which use that, whereas I wonder if we do this in Gateway API if it will be there just for us for a prolonged period of time. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants