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

Chore: Extracted the dns monitor to its own monitor-type #3413

Merged
merged 12 commits into from
Sep 9, 2023

Conversation

CommanderStorm
Copy link
Collaborator

@CommanderStorm CommanderStorm commented Jul 12, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

This PR lifts the DNS-monitor from the gigantic monitor.js into its own monitor-type

Type of change

  • Other

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

image

@CommanderStorm CommanderStorm changed the title Extracted the dns monitor to its own monitor-type Refactoring: Extracted the dns monitor to its own monitor-type Jul 12, 2023
@CommanderStorm CommanderStorm changed the title Refactoring: Extracted the dns monitor to its own monitor-type Chore: Extracted the dns monitor to its own monitor-type Jul 12, 2023
# Conflicts:
#	server/model/monitor.js
#	server/uptime-kuma-server.js
@CommanderStorm CommanderStorm marked this pull request as ready for review July 25, 2023 16:31
@CommanderStorm
Copy link
Collaborator Author

I can test more of the monitors' functionality if requested, but given that this is just a code-move, I don't feel that this would be able to uncover any further bugs. (I have not touched the code at all, just where it lives)

Secondary reason:
It is really hard to find example domains to test against 😅

Copy link
Contributor

@Computroniks Computroniks left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Slight side note, that massive if else block looks like the perfect candidate for a switch block

@CommanderStorm
Copy link
Collaborator Author

Slight side note, that massive if else block looks like the perfect candidate for a switch block

I limited the refactoring to lifting the code to another place
=> Any adjustments like the ones proposed by #432 / ... can be done afterwards

@louislam louislam merged commit d6af916 into louislam:master Sep 9, 2023
@CommanderStorm CommanderStorm deleted the dns_monitor branch September 9, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants