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

Register webhook in codes #332

Merged

Conversation

TommyLike
Copy link
Contributor

@TommyLike TommyLike commented Jul 12, 2019

For Issue #329, Within this change, admission webhook configs as below are registered in admission codes automatically. Therefore this yaml can be removed.

apiVersion: admissionregistration.k8s.io/v1beta1
kind: ValidatingWebhookConfiguration
metadata:
  name: {{ .Release.Name }}-validate-job
  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/hook-delete-policy": before-hook-creation
webhooks:
  - clientConfig:
      service:
        name: {{ .Release.Name }}-admission-service
        namespace: {{ .Release.Namespace }}
        path: /jobs
    failurePolicy: Ignore
    name: validatejob.volcano.sh
    namespaceSelector: {}
    rules:
      - apiGroups:
          - "batch.volcano.sh"
        apiVersions:
          - "v1alpha1"
        operations:
          - CREATE
          - UPDATE
        resources:
          - jobs
---
apiVersion: admissionregistration.k8s.io/v1beta1
kind: MutatingWebhookConfiguration
metadata:
  name: {{ .Release.Name }}-mutate-job
  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/hook-delete-policy": before-hook-creation
webhooks:
  - clientConfig:
      service:
        name: {{ .Release.Name }}-admission-service
        namespace: {{ .Release.Namespace }}
        path: /mutating-jobs
    failurePolicy: Ignore
    name: mutatejob.volcano.sh
    namespaceSelector: {}
    rules:
      - apiGroups:
          - "batch.volcano.sh"
        apiVersions:
          - "v1alpha1"
        operations:
          - CREATE
        resources:
          - jobs

Also, this two configurations have been added the owner reference of admission service, which makes them deleted automatically when purge helm chart.

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2019
@TommyLike
Copy link
Contributor Author

This CI would fail due to the unchange of helm chart.

@TommyLike TommyLike force-pushed the feature/register_webhook_codes branch from 05e4bed to 9c7ae78 Compare July 12, 2019 08:35
@TravisBuddy
Copy link

Travis tests have failed

Hey @TommyLike,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 333d5f20-a480-11e9-bd96-c5bef65e495d

@TommyLike TommyLike force-pushed the feature/register_webhook_codes branch from 9c7ae78 to f911c40 Compare July 12, 2019 09:50
@k82cn
Copy link
Member

k82cn commented Jul 12, 2019

@TommyLike @hzxuzhonghu @asifdxtreme , can you help to list the items we need to do to improve contributor experience for this? That's really hard to get the progress of that if we open PR one by one :(

@TravisBuddy
Copy link

Travis tests have failed

Hey @TommyLike,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: f240e000-a48e-11e9-bd96-c5bef65e495d

@TommyLike
Copy link
Contributor Author

@TommyLike @hzxuzhonghu @asifdxtreme , can you help to list the items we need to do to improve contributor experience for this? That's really hard to get the progress of that if we open PR one by one :(

hmm, @k82cn this patch is not related to contributor experience improvement. @hzxuzhonghu has opened an issue which lists most of the things we need to do to improve it. We can discuss and talk on that one。#325

@k82cn
Copy link
Member

k82cn commented Jul 14, 2019

this patch is not related to contributor experience improvement.

If contributor can not install Volcano, how to contribute?

@TommyLike TommyLike force-pushed the feature/register_webhook_codes branch from f911c40 to 0876e7d Compare July 15, 2019 02:23
@TommyLike
Copy link
Contributor Author

this patch is not related to contributor experience improvement.

If contributor can not install Volcano, how to contribute?

There would be several reasons that leads to failing installing volcano.

  1. kubernetes cluster version requirement.
  2. misconfiguration of the kubernetes cluster
  3. helm binary version requirement.
  4. inconsistency between our charts templates and controller images.
  5. .....

This patch intends to reduce the yaml amount and does not belong to any of these above.

thandayuthapani pushed a commit to thandayuthapani/volcano that referenced this pull request Jul 15, 2019
@TommyLike
Copy link
Contributor Author

/assign @hzxuzhonghu

@TommyLike TommyLike force-pushed the feature/register_webhook_codes branch from 0876e7d to 4482634 Compare July 15, 2019 09:11
@TommyLike TommyLike force-pushed the feature/register_webhook_codes branch from 4482634 to 7418866 Compare July 15, 2019 09:21
@TravisBuddy
Copy link

Travis tests have failed

Hey @TommyLike,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: d4c4c390-a6e4-11e9-a3be-7d74802580dc

@k82cn
Copy link
Member

k82cn commented Jul 15, 2019

/lgtm
/approve

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, TommyLike

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

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019
@volcano-sh-bot volcano-sh-bot merged commit fd3cf10 into volcano-sh:master Jul 15, 2019
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants