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

🌱 Automatically update the multiversion tutorial #4138

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 8, 2024

Automatically update the multiversion tutorial
Fixes: #3880

Closes: #4103

Co-Author: sarthaksarthak9 sarthaknegi908@gmail.com

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 8, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 8, 2024
@camilamacedo86 camilamacedo86 force-pushed the update-generate-multiversion branch 16 times, most recently from 777586c to 46d09a5 Compare September 8, 2024 22:18
@camilamacedo86 camilamacedo86 changed the title WIP Automatically update the multiversion tutorial Sep 8, 2024
@camilamacedo86 camilamacedo86 changed the title Automatically update the multiversion tutorial 🌱 Automatically update the multiversion tutorial Sep 8, 2024
@camilamacedo86 camilamacedo86 changed the title 🌱 Automatically update the multiversion tutorial WIP : 🌱 Automatically update the multiversion tutorial Sep 8, 2024
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2024
@camilamacedo86 camilamacedo86 changed the title WIP : 🌱 Automatically update the multiversion tutorial (Blocked by #4145) : 🌱 Automatically update the multiversion tutorial Sep 8, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 8, 2024
@sarthaksarthak9
Copy link
Contributor

sarthaksarthak9 commented Sep 9, 2024

@camilamacedo86 Thanks. Its look good to me.

/lgtm

@k8s-ci-robot
Copy link
Contributor

@sarthaksarthak9: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

// Example:
// It("Should apply defaults when a required field is empty", func() {
// By("simulating a scenario where defaults should be applied")
// obj.SomeFieldWithDefault = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

odd indentation here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we need to solve that in the scaffold see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook_test_template.go

We need another PR for that. I raised an issue: #4148
Good catcher btw !!!!

// By("simulating a scenario where defaults should be applied")
// obj.SomeFieldWithDefault = ""
// err := obj.Default(ctx)
// Expect(err).NotTo(HaveOccurred())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Expect(err).NotTo(HaveOccurred())
// Expect(obj.Default(ctx)).To(Succeed())

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we need to solve that in the scaffold see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook_test_template.go

We need another PR for that. I raised an issue: #4148
Good catcher btw !!!!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mogsie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

// It("Should deny creation if a required field is missing", func() {
// By("simulating an invalid creation scenario")
// obj.SomeRequiredField = ""
// warnings, err := obj.ValidateCreate(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

These three lines can be replaced with one:

Suggested change
// warnings, err := obj.ValidateCreate(ctx)
// Expect(obj.ValidateCreate(ctx)).Error().To(HaveOccurred())

This assertion includes the non-nil error check and the nil "warnings".

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we need to solve that in the scaffold see: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/api/webhook_test_template.go

We need another PR for that. I raised an issue: #4148
Good catcher btw !!!!

…nerate

All our samples and documentation are automatically updated through the build process.
However, the logic for handling multiversion samples was missing. This commit adds the
necessary implementation to ensure multiversion samples are generated correctly using `make generate`.

Co-Author: sarthaksarthak9 <sarthaknegi908@gmail.com>
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024
@camilamacedo86 camilamacedo86 changed the title (Blocked by #4145) : 🌱 Automatically update the multiversion tutorial 🌱 Automatically update the multiversion tutorial Sep 9, 2024
@camilamacedo86 camilamacedo86 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024
@camilamacedo86 camilamacedo86 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2024
@k8s-ci-robot k8s-ci-robot merged commit 6dd2130 into kubernetes-sigs:master Sep 9, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the Multi-Version tutorial programatically when we run make generate
4 participants