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.

Sign up for GitHub

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

Jump to bottom

Use standard Condition definitions and add a Default derive #30

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Use standard Condition definitions and add a Default derive #30

merged 4 commits into from
Apr 30, 2024

Conversation

aryan9600
Copy link
Contributor

@aryan9600 aryan9600 commented Mar 10, 2024 •
edited
Loading

Regenerate the definitions with kopium v0.17.0 to get rid of the custom Condition definitions generated for each API and use a standard definition from the k8s-openapi crate instead. Also, update the kopium command in update.sh so that all APIs auto-derive the Default trait.

Ref: kube-rs/kopium#203
Fixes #28

shaneutt
shaneutt requested changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

This rocks, and the work you did over in Kopium rocks as well. Thank you for taking care of this 🙇

The only change I was thinking is that we could add something to our integration test in lib.rs. You'll see here:

gateway-api-rs/src/lib.rs

Line 71 in d6f8023

assert!(gw.metadata.uid.is_some());

That we have some asserts on the default contents (metadata, e.t.c.) I was thinking we could just add some asserts that double check the status now too? LMKWYT 🤔

shaneutt
shaneutt approved these changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@shaneutt shaneutt enabled auto-merge (squash) March 19, 2024 21:14
@shaneutt
Copy link
Collaborator

Hey @aryan9600 something seems to be up with CI

@shaneutt shaneutt self-requested a review April 3, 2024 15:18
@shaneutt shaneutt marked this pull request as draft April 3, 2024 15:18
auto-merge was automatically disabled April 3, 2024 15:18

Pull request was converted to draft

@aryan9600 aryan9600 marked this pull request as ready for review April 11, 2024 11:41
shaneutt
shaneutt requested changes Apr 14, 2024
View reviewed changes
Copy link
Collaborator

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Very awesome, thank you for adding this!

I'm sorry it took so long for me to get to a review, it was a busy week.

This is my first pass, check out these comments and let me know your thoughts, and I'll do another pass ASAP.

src/apis/experimental/enum_defaults.rs Outdated Show resolved Hide resolved
tools/enum_default_generator/Cargo.toml Outdated Show resolved Hide resolved
update.sh Show resolved Hide resolved
@shaneutt
Copy link
Collaborator

/cc @astoycos

@aryan9600
use standard Condition definitions from k8s-openapi
943e0ee 
Regenrate the definitions with kopium v0.17.2 to get rid of the custom
Condition definitions generated for each API and use a standard
definition from the k8s-openapi crate instead.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@aryan9600
add codegen tool to generate Default impls for enums
a669795 
Add a codegen tool `enum_default_generator` that reads enum names along
with their default variants from the enviornment and then generates
implementations of the Default trait for them.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@aryan9600
derive the Default trait for all APIs
a77cbc0 
Modify `update.sh` to generate APIs which derive the `Default` trait:

* Update the kopium command in `update.sh` so that all APIs auto-derive
  the Default trait.
* Use the enum default generator tool to generate `Default` trait impls
  for all Gateway API enums.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@aryan9600
refactor enum default generator as an xtask
c97c721 
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@aryan9600 aryan9600 requested a review from shaneutt April 24, 2024 14:59
@shaneutt
Copy link
Collaborator

@aryan9600 you should be able to use git mv to move all the previous code into the sub-directory while still maintaining git attribution and history. Something like mkdir gateway-api && git mv * gateway-api?

shaneutt
shaneutt approved these changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Talked about this in Slack: while there are some advantages to using git mv (keeping history, keeping attribution, providing a diff of the new generated changes) it also takes quite a bit of time so I'm fine with us not worrying about that for now.

@shaneutt shaneutt merged commit 298804f into kube-rs:main Apr 30, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaneutt shaneutt shaneutt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Condition from k8s-openapi crate for condition
2 participants
@aryan9600 @shaneutt