feat(stackable-versioned): Use attribute instead of derive macro

[Techassi]

Description

Tracked by stackabletech/issues#507, follow-up of #764

Attribute macros allow generating code in place instead of appending code when using derive macros. This enables us to completely generate code from scratch and avoids that the "base" container is marked as dead code (among other advantages).

Reviewer

Preview Give feedback

1. Code contains useful comments
2. (Integration-)Test cases added
3. Documentation added or updated
4. Changelog updated
5. Cargo.toml only contains references to git tags (not specific commits or branches)

[NickLarsenNZ]

I think a module should be auto-generated for the container, which then contains the versions of the container. Eg:

• snake_case the container name
• PascalCase the versions within

#[versioned(
  version(name = "v1alpha1"),
  version(name = "v1beta1"),
)]
pub struct Foo {
  #[versioned(deprecated(since = "v1beta1"))]
  bar: String,
  baz: String,
}

// produces...

pub mod foo {
  pub struct V1Alpha1 {
    bar: String,
    baz: String,
  }

  pub struct V1Beta1 {
    #[deprecated]
    deprecated_bar: String,
    baz: String,
  }
}

[sbernauer]

I have to admit I prefer @NickLarsenNZ's suggestion. However, I could also imagine the following for better readability

#[versioned(name = "Foo", version(name = "v1alpha1"), version(name = "v1beta1"))]
pub struct FooVersions {
    #[versioned(deprecated(since = "v1beta1"))]
    bar: String,
    baz: String,
}

// generates

#[allow(non_camel_case_types)]
pub struct Foo_v1alpha1 {
    bar: String,
    baz: String,
}

#[allow(non_camel_case_types)]
pub struct Foo_v1beta1 {
    #[deprecated]
    deprecated_bar: String,
    baz: String,
}

// To make it easier to read and pass the object around in operator-code (always points to latest version)
type Foo = Foo_v1beta1;

[Techassi]

Just using struct names (without any modules) was also on the table, but rejected after some discussions before stackabletech/issues#507 took final shape.

I also really like the type alias. That's why it is included since c8e9a5c. The type alias is possible with both approaches:

• Module named after version, container named as is: pub type Foo = v1::Foo
• Module named after container, container named after version: pub type Foo = foo::V1

Apart from that, the currently generated code looks like this:

#[versioned(
    version(name = "v1alpha1"),
    version(name = "v1beta1"),
    version(name = "v1"),
    version(name = "v2"),
    version(name = "v3")
)]
struct Foo {
    /// My docs
    #[versioned(
        added(since = "v1alpha1"),
        renamed(since = "v1beta1", from = "jjj"),
        deprecated(since = "v2", note = "not empty")
    )]
    deprecated_bar: usize,
    baz: bool,
}

// Produces ...

#[automatically_derived]
pub mod v1alpha1 {
    pub struct Foo {
        pub jjj: usize,
        pub baz: bool,
    }
}

#[automatically_derived]
pub mod v1beta1 {
    pub struct Foo {
        pub bar: usize,
        pub baz: bool,
    }
}

#[automatically_derived]
pub mod v1 {
    pub struct Foo {
        pub bar: usize,
        pub baz: bool,
    }
}

#[automatically_derived]
pub mod v2 {
    pub struct Foo {
        #[deprecated = "not empty"]
        pub deprecated_bar: usize,
        pub baz: bool,
    }
}

#[automatically_derived]
pub mod v3 {
    pub struct Foo {
        // Missing #[deprecated] will be addressed
        pub deprecated_bar: usize,
        pub baz: bool,
    }
}

pub type Foo = v3::Foo;

[Techassi]

I have to admit I prefer @NickLarsenNZ's suggestion.

I have no strong feelings for either of both options, but I agree that

A container Foo has a set of versions. A version does not have a set of containers (eg: Foo and Bar).
Quoted from stackabletech/issues#507 (comment)

makes indeed a lot of sense.

[Techassi]

This PR is ready for review.

I would prefer to merge this as is, and change the code generation in a separate PR (if required) once we reach a decision.

[NickLarsenNZ]

However, I could also imagine the following for better readability
...

pub struct Foo_v1alpha1 {

To me, the Foo_ prefix is acting as a namespace... which is basically what a mod is for.

[NickLarsenNZ]

LGTM

[NickLarsenNZ]

I would prefer to merge this as is, and change the code generation in a separate PR (if required) once we reach a decision.

I would consider the module-struct inversion to be required. Happy for that to be done in a separate PR though.

[Techassi]

I opened #797 to change the generated module and container names as discussed in this PR.

[nightkr]

I'd disagree on both counts. The typical case is that you are working against one version of a given API, so it makes sense to optimize for naming that makes sense when imported. v1alpha1::Foo and Foo (with an earlier use v1alpha1::Foo;) are both meaningful names, V1Alpha1 (with use foo::V1Alpha1) is not.

I also generally think that the "current version" aliases are an antifeature, use v1alpha1::*; provides the same experience while making upgrades an explicit choice by the downstream code.

[Techassi]

I also generally think that the "current version" aliases are an antifeature, use v1alpha1::*; provides the same experience while making upgrades an explicit choice by the downstream code.

In case we stick with the current naming scheme, aka v1alpha1::Foo, what is your opinion on introducing a latest::Foo "alias", aka an additional latest module pointing to the latest version of Foo:

pub mod v1alpha1 {
  pub struct Foo;
}

pub mod latest {
  pub use v1alpha1::Foo;
}

[NickLarsenNZ]

I'd disagree on both counts. The typical case is that you are working against one version of a given API, so it makes sense to optimize for naming that makes sense when imported.

Typically, yes, but it still seems backward IMO.

v1alpha1::Foo and Foo (with an earlier use v1alpha1::Foo;) are both meaningful names, V1Alpha1 (with use foo::V1Alpha1) is not.

Only if importing it that way, and I don't think that is a strong enough reason.
It can still make sense using the relative qualified name of foo::V1Alpha1.

I also generally think that the "current version" aliases are an antifeature, use v1alpha1::*; provides the same experience while making upgrades an explicit choice by the downstream code.

I'm ok with the latest module alias as @Techassi suggests below. Let the compiler guide the developer to the required changes (one less change). But I don't mind if it isn't there, explicit is also fine.

---

EDIT: I also think we need to consider it used outside of stackabletech (reusable OSS code) and not just tailor it for our use case.

[Techassi]

I'll put this up as a decision. You can discuss and vote below.

The currently considered options are:

• 🚀 foo::V1Alpha1
• ❤️ v1alpha1::Foo
• 👀 Foo_V1Alpha1

Please also use 👍 if you would like to see an alias, like type Foo = <TBD>, foo::Latest, or latest::<TBD>.

[sbernauer]

Just for the record: k8s-openapi is using v1::HorizontalPodAutoscaler and v2::HorizontalPodAutoscaler here

[nightkr]

Typically, yes, but it still seems backward IMO.

I think part of this is a question of mindset: is this version for the specific resource/kind ("Deployment v2 in the apps group") or for the whole API group ("Deployment in v2 of the apps group")?

Mechanically, Kubernetes tracks versions separately for each CRD, but the "end-user API" (that is, everything except for the CustomResourceDefinition API itself) encourages thinking of each version as covering the whole API group.

For example, you define:

apiVersion: apps/v1
kind: Deployment

rather than

apiGroup: apps
kind: Deployment/v1

In case we stick with the current naming scheme, aka v1alpha1::Foo, what is your opinion on introducing a latest::Foo "alias", aka an additional latest module pointing to the latest version of Foo:

My argument applies the same, regardless of what you call the alias.. :P

My point is that upgrading to the latest API version should be an explicit choice that is made and tested, not something that happens automatically as part of a routine dependency bump.

If it was a simple routine upgrade that didn't take intervention then it probably didn't need the (API) version bump at all!

[Techassi]

It was decided to stick with the current naming scheme (v1alpha1::Foo).

[NickLarsenNZ]

Typically, yes, but it still seems backward IMO.

I think part of this is a question of mindset: is this version for the specific resource/kind ("Deployment v2 in the apps group") or for the whole API group ("Deployment in v2 of the apps group")?

In that case, the current implementation of stackable-versioned doesn't enforce that constraint.
Assuming they are in the same api group, shouldn't both structs have to change?

#[versioned(
    version(name = "v1alpha1"),
    version(name = "v1beta1"),
)]
struct Foo {
    foo: bool,
}

#[versioned(
    version(name = "v1alpha1"),
    // No v1beta1 declaration here
)]
struct Bar {
    bar: String,
}