Closed Techassi closed 2 months ago
I think a module should be auto-generated for the container, which then contains the versions of the container. Eg:
#[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,
}
}
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;
Just using struct names (without any modules) was also on the table, but rejected after some discussions before https://github.com/stackabletech/issues/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:
pub type Foo = v1::Foo
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;
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 https://github.com/stackabletech/issues/issues/507#issuecomment-2126664396
makes indeed a lot of sense.
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.
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.
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.
I opened #797 to change the generated module and container names as discussed in this PR.
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.
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;
}
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
andFoo
(with an earlieruse v1alpha1::Foo;
) are both meaningful names,V1Alpha1
(withuse 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.
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 :+1: if you would like to see an alias, like type Foo = <TBD>
, foo::Latest
, or latest::<TBD>
.
Just for the record: k8s-openapi
is using v1::HorizontalPodAutoscaler
and v2::HorizontalPodAutoscaler
here
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!
It was decided to stick with the current naming scheme (v1alpha1::Foo
).
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,
}
Description
Tracked by https://github.com/stackabletech/issues/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).