Open
Conversation
Signed-off-by: Lee Briggs <lee@leebriggs.co.uk>
Signed-off-by: Lee Briggs <lee@leebriggs.co.uk>
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 6ab0074 in 1 minute and 41 seconds. Click for details.
- Reviewed
464lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. LICENSE:21
- Draft comment:
Missing newline at EOF; consider adding one for POSIX compliance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While POSIX compliance for newlines at EOF is a real thing, this is a LICENSE file that's being added. The missing newline won't affect functionality. Many tools automatically handle this. It's a very minor style issue that doesn't warrant a comment, especially on a standard license file. The POSIX standard does recommend newlines at EOF, and some tools might have issues without it. Could this cause problems with license scanning tools? Even if some tools prefer newlines at EOF, most modern tools handle files without them just fine. The content and validity of the MIT license is not affected. Delete this comment as it's too minor of an issue to warrant attention, especially for a standard license file.
2. cmd.go:50
- Draft comment:
Include 'pihole' in dns-provider flag and ensure documentation and examples are updated. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. cmd.go:58
- Draft comment:
New Pi-hole flags added; verify that the bool flag 'pihole-tls-insecure-skip-verify' is correctly handled and documented. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify that a new flag is correctly handled and documented. This falls under asking the author to ensure something is done, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue with the code.
4. providers/pihole.go:41
- Draft comment:
Confirm that trimming the trailing slash from baseURL is acceptable for all Pi-hole deployments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding a specific change, which violates the rule against asking for confirmation of intention. It does not provide a specific code suggestion or ask for a test to be written.
5. providers/pihole.go:169
- Draft comment:
If multiple duplicate records exist, consider deleting all matches instead of breaking after the first deletion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a valid concern - if multiple records with the same name and type exist, we're only deleting the first one. However, in DNS systems, having multiple identical records is usually an invalid state. The code is handling the common case correctly. Without knowing the Pi-hole API details, we can't be certain if multiple duplicates are even possible. I may be underestimating the importance of handling duplicate records. If the Pi-hole API allows duplicates, leaving them could cause issues. While handling duplicates could be more robust, suggesting a change without clear evidence that duplicates are possible or problematic violates the rule about making speculative comments. Delete the comment since it's speculative without clear evidence that duplicate records are possible or problematic in Pi-hole's DNS implementation.
6. cmd.go:204
- Draft comment:
The comment here uses "PiHole" while elsewhere it uses "Pi-hole". Consider using a consistent naming convention for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the inconsistency, it's a very minor documentation issue that doesn't affect functionality. The code itself uses the hyphenated form consistently in all variable names and config keys. This is just about a comment style preference. According to our rules, we should not make purely informative comments or comments about obvious/unimportant issues. The inconsistency could potentially confuse users when they're searching documentation or error messages. Consistent naming in documentation is a best practice. While documentation consistency is good, this is an extremely minor issue that doesn't impact usability or code quality. The official product name "Pi-hole" is used correctly in all user-facing elements like flags and config keys. Delete this comment as it's too minor of an issue and doesn't require code changes. It's just about documentation style.
Workflow ID: wflow_9AszNBG9JKxQ6V1Q
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| return fmt.Errorf("dns.cloudflare.api_token is required when using cloudflare provider") | ||
| } | ||
| case "pihole": | ||
| if c.DNS.Pihole.BaseURL == "" { |
There was a problem hiding this comment.
Consider validating that 'pihole.base_url' is a properly formatted URL (e.g., starts with http:// or https://).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rough thoughts about HTTPS support for Pihole v6
Might need to make the ignore TLS self-signed optional for folks with real DNS implementations
Important
Adds Pi-hole support as a DNS provider with optional TLS verification for self-signed certificates in DNSScale.
cmd.go,config.go, andmain.go.pihole-tls-insecure-skip-verifyflag to optionally skip TLS verification for self-signed certificates.PiholeProviderinproviders/pihole.goto manage DNS records via Pi-hole API.cmd.goto include Pi-hole specific flags and environment variables.config.goto addPiholeConfigstruct for Pi-hole settings.LICENSEfile with MIT License.README.mdto reflect Pi-hole support and license information.This description was created by
for 6ab0074. You can customize this summary. It will automatically update as commits are pushed.