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

Introduce managedBy field and Remove managed-by label #487

Merged
merged 22 commits into from Mar 29, 2024
Merged

Introduce managedBy field and Remove managed-by label #487

merged 22 commits into from Mar 29, 2024

Conversation

jedwins1998
Copy link
Contributor

This pull request is to introduce managedBy field and Remove managed-by label.

Original Issue: #440

@jedwins1998
Remove in const declaration and add field to JobSetSpec.
cd66b3e
@jedwins1998
Remove references to LabelManagedBy in jobset_webhook.go.
8c45f05
@jedwins1998
Add struct tag for ManagedBy field of JobSetSpec.
7820c47
@jedwins1998
Add missing ` character in ManagedBy struct tag.
83ee86f
@jedwins1998
Update jobset_webhook_test.go to remove references to LabelManagedBy.
0713346
@jedwins1998
Update Reconcile to use ManagedBy field of JobSetSpec.
e866ed4
@jedwins1998
Update jobset_controller_test.go to use ManagedBy field of JobSetSpec.
34c2b26
@jedwins1998
Update jobset_webhook_test.go to use ManagedBy field of JobSetSpec.
ef199e7
@jedwins1998
Fix bug where uninitialized value for js.Spec.ManagedBy was treated a…
55fb065 
…s empty string "".
@jedwins1998
Add an integration test that only tests job creation.
6d3f0a0
@jedwins1998
Ran non-test related make commands.
80643db
@jedwins1998
Correct grammatical error.
c3f9aef
@jedwins1998
Regenerated files.
0db3446
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Mar 28, 2024 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot
Copy link
Contributor

Welcome @jedwins1998!

It looks like this is your first PR to kubernetes-sigs/jobset 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/jobset has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 28, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @jedwins1998. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 28, 2024 •
edited

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name Link
🔨 Latest commit 80ac00c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/66073f1d35e5080009772ee1

@jedwins1998 jedwins1998 mentioned this pull request Mar 28, 2024
Closed
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 28, 2024
@kannon92
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 28, 2024
danielvegamyhre
danielvegamyhre reviewed Mar 29, 2024
View reviewed changes
Copy link
Contributor

@danielvegamyhre danielvegamyhre left a comment

Choose a reason for hiding this comment

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

LGTM overall, left one minor comment

pkg/controllers/jobset_controller.go Outdated Show resolved Hide resolved
@danielvegamyhre
Copy link
Contributor

Let's also validate the ManagedBy field like is done upstream here https://github.com/kubernetes/kubernetes/blob/f4e246bc93ffb68b33ed67c7896c379efa4207e7/pkg/apis/batch/validation/validation.go#L220-L223

mimowo
mimowo reviewed Mar 29, 2024
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

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

Question regarding validation, other than that lgtm,

api/jobset/v1alpha2/jobset_types.go
JobSetManager = "jobset"
// JobSetControllerName is the reserved value for the managedBy field for the built-in
// JobSet controller.
JobSetControllerName = "jobset.sigs.k8s.io/jobset-controller"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we to introduce validation in the webhook with IsDomainPrefixedPath, analogously as for Job: https://github.com/kubernetes/kubernetes/blob/f4e246bc93ffb68b33ed67c7896c379efa4207e7/pkg/apis/batch/validation/validation.go#L220-L223. Potentially also length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think so, we have been bitten before by not having strict enough validation, so I'd like to enforce the field is restricted to expected format and length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code. Please resolve if nothing else is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add unit tests ensuring the validation works as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jedwins1998 jedwins1998 requested a review from mimowo March 29, 2024 20:04
@danielvegamyhre
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielvegamyhre, jedwins1998

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9348439 into kubernetes-sigs:main Mar 29, 2024
12 checks passed
@mimowo mimowo mentioned this pull request Apr 2, 2024
Merged
@danielvegamyhre danielvegamyhre mentioned this pull request Apr 10, 2024
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kannon92 kannon92 Awaiting requested review from kannon92

@danielvegamyhre danielvegamyhre Awaiting requested review from danielvegamyhre

@mimowo mimowo Awaiting requested review from mimowo

Assignees

@danielvegamyhre danielvegamyhre

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jedwins1998 @k8s-ci-robot @kannon92 @danielvegamyhre @mimowo