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

Add secondary ingress #63

Merged
merged 7 commits into from
Sep 22, 2020
Merged

Conversation

halkeye
Copy link
Member

@halkeye halkeye commented Sep 18, 2020

What this PR does / why we need it

Which issue this PR fixes

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • CHANGELOG.md was updated

Signed-off-by: Gavin Mogan <[email protected]>
Signed-off-by: Gavin Mogan <[email protected]>
@halkeye halkeye marked this pull request as ready for review September 21, 2020 01:17
@halkeye halkeye requested a review from a team as a code owner September 21, 2020 01:17
@halkeye
Copy link
Member Author

halkeye commented Sep 22, 2020

tested via:

---
# Source: jenkins/templates/jenkins-master-secondary-ingress.yaml
apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  namespace: media
  labels:
    "app.kubernetes.io/name": 'jenkins'
    "helm.sh/chart": "jenkins-2.6.5"
    "app.kubernetes.io/managed-by": "Helm"
    "app.kubernetes.io/instance": "RELEASE-NAME"
    "app.kubernetes.io/component": "jenkins-master"
    labela: valuea
    labelb: valueb
  annotations:
    annoitationa: valuea
    annoitationb: valueb
  name: RELEASE-NAME-jenkins-secondary
spec:
  rules:
    - host: something
      http:
        paths:
          - path: "/github-webhook"
            backend:
              serviceName: RELEASE-NAME-jenkins
              servicePort: 8080
  tls:
    - hosts:
      - jenkins-external.example.com
      secretName: jenkins-external.example.com
 cat value2.yaml
master:
  ingress:
    enabled: false
    hostName: 'foo'
  secondaryingress:
    enabled: true
    # paths you want forwarded to the backend
    # ex /github-webhook
    paths:
      - /github-webhook
    # For Kubernetes v1.14+, use 'networking.k8s.io/v1beta1'
    apiVersion: "extensions/v1beta1"
    labels:
      labela: valuea
      labelb: valueb
    annotations:
      annoitationa: valuea
      annoitationb: valueb
    # kubernetes.io/ingress.class: nginx
    # kubernetes.io/tls-acme: "true"
    # configures the hostname e.g. jenkins-external.example.com
    hostName: something
    tls:
      - secretName: jenkins-external.example.com
        hosts:
          - jenkins-external.example.com

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

@@ -1,7 +1,7 @@
apiVersion: v1
name: jenkins
home: https://jenkins.io/
version: 2.6.4
version: 2.6.5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
version: 2.6.5
version: 2.7.0

Signed-off-by: Gavin Mogan <[email protected]>
enabled: false
# paths you want forwarded to the backend
# ex /github-webhook
paths: []
Copy link
Member

Choose a reason for hiding this comment

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

what about putting the github webhook in here by default commented out?

I suspect that would be what it's used for the majority of the time

Suggested change
paths: []
paths: []
#paths:
# - /github-webhook

Copy link
Member Author

Choose a reason for hiding this comment

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

does helm merge or overwrite arrays?
if its merge, that would mean there's no way to disable it

What about the straight up non github webhook? I think its /notifyCommit
What about prefixes? by making it need to be configured, its able to be prefixed, the default wouldn't be.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, it overwrites a list, so it would be doable.

Copy link
Member

Choose a reason for hiding this comment

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

I had it commented out to avoid those issues 👅

Copy link
Member Author

Choose a reason for hiding this comment

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

oooh i get what your saying, i can change it if desired when i'm on a computer again

Copy link
Member

Choose a reason for hiding this comment

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

is fine 🚢

@timja timja added the enhancement New feature or request label Sep 22, 2020
@timja timja merged commit 783d670 into jenkinsci:main Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add secondary ingress to allow webhooks to be public
3 participants