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

remove pointers from types, add webhook validation for service #583

Merged
merged 5 commits into from
Apr 5, 2018

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Apr 4, 2018

Addresses part of #412

Proposed Changes

  • Add validation for Service objects in webhook
  • Change RunLatest.Configuration and Pinned.Configuration to not be pointers, they are not optional so shouldn't be pointers.

@google-prow-robot google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 4, 2018
@vaikas
Copy link
Contributor Author

vaikas commented Apr 4, 2018

/assign @evankanderson

)

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// Service
type Service struct {
metav1.TypeMeta `jsonL",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
metav1.TypeMeta `json",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be json:",inline"?

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.

version: v1alpha1
names:
kind: Service
plural: services
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this affect kubectl get services? Does it override the core Service resource?

Copy link
Contributor Author

@vaikas vaikas Apr 4, 2018

Choose a reason for hiding this comment

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

It doesn't. You have to use the full name for the ela service:

$ kubectl get services
NAME                           TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)          AGE
git-webhook-00001-autoscaler   NodePort    10.11.247.70    <none>        8080:30112/TCP   13d
git-webhook-00001-service      NodePort    10.11.240.64    <none>        80:30278/TCP     13d
git-webhook-service            ClusterIP   10.11.251.209   <none>        80/TCP           13d
kubernetes                     ClusterIP   10.11.240.1     <none>        443/TCP          33d
vaikas@vaikas-linux3:~$ kubectl get services.elafros.dev
No resources found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks!

@vaikas
Copy link
Contributor Author

vaikas commented Apr 4, 2018

Thanks, PTAL :)

Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2018
t.Errorf("Expected success, but failed with: %s", err)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

TestRunLatestWithMissingConfiguration?

@google-prow-robot google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2018
@vaikas
Copy link
Contributor Author

vaikas commented Apr 5, 2018

Thanks, PTAL. test added.

@evankanderson
Copy link
Member

/lgtm
/approve

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2018
@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, grantr, vaikas-google

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:
  • OWNERS [evankanderson,vaikas-google]

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

@vaikas vaikas merged commit c1c7025 into knative:master Apr 5, 2018
@vaikas vaikas deleted the steve-validator branch April 5, 2018 21:30
markusthoemmes referenced this pull request in openshift/knative-serving Apr 3, 2019
* remove pointers from types, add webhook validation for service

* add service.yaml file so the CRD is created

* remove errorneous go_library

* fix typo

* add a test for validating Service with missing RunLatest.Configuration fails
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. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants