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

ingress paths are now clickable #7487

Merged
merged 2 commits into from
Dec 2, 2022

Conversation

marcosdiez
Copy link
Contributor

This is a copy of #7017
163737717-08e42793-e9e9-4ba5-b349-4a9390c59af4


sample YAML to test

kind: Ingress
apiVersion: networking.k8s.io/v1
metadata:
  name: kubernetes-dashboard-lb2
  namespace: kubernetes-dashboard
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/backend-protocol: HTTPS
spec:
  defaultBackend:
    service:
      name: test
      port:
        number: 80
  rules:
    - host: dashboard.mydomain.com
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: kubernetes-dashboard
                port:
                  number: 443
    - host: dashboard2.mydomain.com
      http:
        paths:
          - path: /mypath
            pathType: Prefix
            backend:
              service:
                name: kubernetes-dashboard
                port:
                  number: 443
    - host: dashboard2.mydomain.com
      http:
        paths:
          - path: /mypath/morepath
            pathType: Prefix
            backend:
              service:
                name: kubernetes-dashboard
                port:
                  number: 443
    - http:
        paths:
        - path: /testpath
          pathType: Prefix
          backend:
            service:
              name: test
              port:
                number: 80
    - http:
        paths:
          - path: /icons
            pathType: ImplementationSpecific
            backend:
              resource:
                apiGroup: k8s.example.com
                kind: StorageBucket
                name: icon-assets
    - host: "foo.bar.com"
      http:
        paths:
          - pathType: Prefix
            path: "/bar"
            backend:
              service:
                name: service1
                port:
                  number: 80
    - host: "*.foo.com"
      http:
        paths:
          - pathType: Prefix
            path: "/foo"
            backend:
              service:
                name: service2
                port:
                  number: 80
    - host: foo.bar.com
      http:
        paths:
          - path: /foo
            pathType: Prefix
            backend:
              service:
                name: service1
                port:
                  number: 4200
          - path: /bar
            pathType: Prefix
            backend:
              service:
                name: service2
                port:
                  number: 8080

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 29, 2022
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 29, 2022
{{ !!ingressRuleFlat?.path?.path ? ingressRuleFlat?.path?.path : '-' }}
<ng-container *ngIf="ingressRuleFlat.path.path">
<ng-container *ngIf="ingressRuleFlat.host">
<a href="https://{{ ingressRuleFlat.host }}{{ ingressRuleFlat?.path?.path }}"

Check warning

Code scanning / CodeQL

Potentially unsafe external link

External links without noopener/noreferrer are a potential security risk.
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to use ?. as *ngIf above already checks that. I think it should be in the first changed line.

Copy link
Member

Choose a reason for hiding this comment

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

I'd actually move this logic to the typescript and use a single method to handle path.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that would be the best.

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 removed the ?s.

I considered creating a typescript method (below), but I am not sure we want a method returning raw HTML.

I can change that if you guys prefer, though.

  createIngressLink(ingressRuleFlat: IngressRuleFlat): string {
    if (ingressRuleFlat.path.path) {
      if (ingressRuleFlat.host) {
        return '<a href="https://' + ingressRuleFlat.host + ingressRuleFlat.path.path + ' target="_blank">' + ingressRuleFlat.path.path + '</a>';
      } else {
        return ingressRuleFlat.path.path;
      }
    } else {
      return "-";
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

Use one method for URL, second for label.

Copy link
Member

Choose a reason for hiding this comment

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

It could also return a simple wrapper object with label and URL fields.

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #7487 (cf9248d) into master (7c78264) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   42.07%   42.09%   +0.02%     
==========================================
  Files         217      217              
  Lines       12084    12084              
  Branches      179      179              
==========================================
+ Hits         5084     5087       +3     
+ Misses       6716     6712       -4     
- Partials      284      285       +1     

@maciaszczykm
Copy link
Member

@marcosdiez Any updates?

@marcosdiez marcosdiez force-pushed the ingress_rule_path_v2 branch 2 times, most recently from 3d1c980 to f441fb6 Compare October 9, 2022 05:15
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 9, 2022
@marcosdiez
Copy link
Contributor Author

@maciaszczykm ready to merge!

remove unecessary question marks

make links safer
@marcosdiez marcosdiez force-pushed the ingress_rule_path_v2 branch from f441fb6 to 007482b Compare October 29, 2022 09:46
Copy link
Member

@maciaszczykm maciaszczykm left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 2, 2022
Copy link
Member

@maciaszczykm maciaszczykm left a comment

Choose a reason for hiding this comment

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

There is no need to use ?. as *ngIf above already checks that. I think it should be in the first changed line.

Actually, wait.

There should be ?. in the first *ngIf: ingressRuleFlat?.path?.path.

Same for last *ngIf you've added: !ingressRuleFlat?.path?.path.

Additionally, to simplify syntax you could use switch instead of ifs or move the code to component like @floreks suggested.

/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2022
@maciaszczykm
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maciaszczykm, marcosdiez

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 merged commit 437d055 into kubernetes:master Dec 2, 2022
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. 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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants