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 Annotations for common knative validation. #37

Closed
wants to merge 11 commits into from
Closed

Use Annotations for common knative validation. #37

wants to merge 11 commits into from

Conversation

n3wscott
Copy link
Contributor

@n3wscott n3wscott commented Aug 2, 2018

Here is the start of me looking at replacing some of the validation we do with annotations.

Please let me know if you like this direction of if I should stop.

@google-prow-robot google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2018
@google-prow-robot google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 2, 2018
@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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

@n3wscott
Copy link
Contributor Author

n3wscott commented Aug 3, 2018

Alternates: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/#advanced-topics Heading: Validation

We could use OpenAPI to do validation. But I think the downside is it is all or nothing with OpenAPI.

@n3wscott
Copy link
Contributor Author

n3wscott commented Aug 3, 2018

/hold

@n3wscott to make a PR with the same functionality but as an OpenAPI config.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2018
@n3wscott n3wscott changed the title [WIP] Looking into Annotations on the Struct for validation. Looking into Annotations on the Struct for validation. Sep 15, 2018
@knative-prow-robot knative-prow-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 15, 2018
@n3wscott
Copy link
Contributor Author

/hold cancel

I think this is still a good idea...

/assign @mattmoor

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2018
imjasonh
imjasonh reviewed Sep 15, 2018
View reviewed changes
apis/validators/k8s_qualified_name.go Outdated
return true
}

func (v *K8sQualifiedNameValidator) Validate(fieldName string, value interface{}) (bool, *apis.FieldError) {
Copy link
Member

Choose a reason for hiding this comment

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

Could Validate just return err, and absence of error means it's valid?

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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

@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 24, 2018
@n3wscott
Copy link
Contributor Author

/test pull-knative-pkg-go-coverage

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-pkg-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
apis/validators/default.go Do not exist 60.0%
apis/validators/k8s_qualified_name.go Do not exist 81.8%
apis/validators/one_of.go Do not exist 97.0%
apis/validators/required.go Do not exist 87.5%
apis/validators/tags.go Do not exist 100.0%
apis/validators/validate.go Do not exist 97.1%

@n3wscott n3wscott changed the title Looking into Annotations on the Struct for validation. Use Annotations for common knative validation. Sep 24, 2018
@imjasonh imjasonh mentioned this pull request Oct 5, 2018
Merged
@n3wscott
Copy link
Contributor Author

I am gonna close this now. Learning about annotations in go was interesting but the inability to annotate internal fields from refed objects makes this not useful.

@n3wscott n3wscott closed this Oct 30, 2018
chengjingtao pushed a commit to katanomi/knative-pkg that referenced this pull request Oct 22, 2024
@danielfbm
feat: Add PropagateCondition to apis/meta/v1alpha1 for lifecycle (kna…
374153f 
…tive#37)

- Adds PropagateCondition function to simplify lifecycle code
- Adds ConditionBuilder for unit tests
chengjingtao pushed a commit to katanomi/knative-pkg that referenced this pull request Oct 22, 2024
auto-sync repository files - chore: add git hook config (knative#37)
af2e8bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjasonh imjasonh imjasonh left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

@vaikas vaikas Awaiting requested review from vaikas

Assignees

@mattmoor mattmoor

Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@n3wscott @google-prow-robot @knative-prow-robot @knative-metrics-robot @imjasonh @mattmoor