- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 6.9k
 
feat: add an tls option to the tcp-port monitor #5678
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
base: master
Are you sure you want to change the base?
Conversation
9b1e919    to
    e9fa79e      
    Compare
  
            
          
                src/pages/EditMonitor.vue
              
                Outdated
          
        
      | <option value="port"> | ||
| TCP Port | ||
| </option> | ||
| <option value="tls"> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go back a step:
how is this different from TCP Port?
Why can this not be integrated/merged with said monitor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. We can probably integrate things with this monitor. I think I was afraid of breaking something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello
I rebased on master and pushed a new proposal. I kept the old commit, but once the proposal is approved, I will squash them. The behavior is slightly different because the probe will not go down when the certificate is about to expire. It will simply send a notification, as with other probes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the proposal is approved, I will squash them
no need to squash (please don't). Makes reviewing a bit harder as I need to do this from scratch every time if you do.
The behavior is slightly different because the probe will not go down when the certificate is about to expire. It will simply send a notification, as with other probes.
If it is indeed expired or non responsive, it should go down. That would be a bug.
5be2b20    to
    2e2acbd      
    Compare
  
    TCP tls monitor| 
           Hello, I just pushed a new version addressing all your remarks. I hope it’s all good now.  | 
    
| 
           Hello, Could I please have an update regarding this PR? Thank you.  | 
    
| 
           the update to this PR is that it needs looking at. It is on the to-review list. Reviewer capacity from my side is severly limited this semester - I am taking 38 ECTS and have a working student job.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7bac05b    to
    1c69b24      
    Compare
  
    5d195f3    to
    179d959      
    Compare
  
    | 
           Hello, Could I please have an update regarding this PR? Thank you.  | 
    
| 
           I am late to this pr. Tested the feature is working fine, just the behaviour of  HTTPS/HTTP Monitor's   
 This pr's  
 Since in http montior, we have the protocol  Remark for testing: https://badssl.com/ with port 443.  | 
    

Tick the checkbox if you understand [x]:
Description
Add a montior to check tls certificate on TCP connexion with others protocols thant HTTP.
Type of change
Checklist
Screenshots
Monitor Configuration
Monitor Résults when it's ok
Change settings of certificate notification
Résult when monitor failed