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 support for Loopia DNS challenge #3821

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

agnon
Copy link
Contributor

@agnon agnon commented Nov 10, 2024

Adds support for Loopia DNS challenges using this great plugin https://pypi.org/project/certbot-dns-loopia/ 1.0.1

Did change versions like others have before me. Not sure if i'm supposed to :) The versioning implies a patch whereas i would argue added functionality is a minor.

Anyways, thanks for a great tool.

Cheers

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for Loopia DNS, allowing users to manage DNS records for domain validation.
    • Updated documentation to include Loopia as a supported DNS provider with necessary configuration details.
  • Configuration Updates

    • New configuration fields for Loopia DNS credentials (loopia_username, loopia_password) added to configuration files.
    • Updated provider list to include dns-loopia.
  • Changelog

    • Updated to reflect the addition of Loopia DNS support in version 5.2.4.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @agnon

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft November 10, 2024 00:49
@agnon agnon marked this pull request as ready for review November 10, 2024 00:51
@agnon
Copy link
Contributor Author

agnon commented Nov 10, 2024

I just noticed there's another PR active for this #3796 but without a signed cla.

Copy link
Contributor

coderabbitai bot commented Nov 10, 2024

📝 Walkthrough
<details>
<summary>📝 Walkthrough</summary>

## Walkthrough
The changes in this pull request introduce support for the Loopia DNS provider in the Letsencrypt project. This includes updates to the changelog, documentation, Dockerfile, configuration files, and scripts to accommodate the new DNS provider. Key additions include new configuration fields for Loopia credentials, a new build argument for the Loopia DNS plugin, and modifications to scripts to handle the new provider's requirements.

## Changes

| File                                      | Change Summary                                                                                   |
|-------------------------------------------|-------------------------------------------------------------------------------------------------|
| letsencrypt/CHANGELOG.md                  | Added new version entry `## 5.2.4` and feature `- Add Loopia DNS support`.                   |
| letsencrypt/DOCS.md                       | Updated documentation to include `dns-loopia` in supported providers and added configuration details. |
| letsencrypt/Dockerfile                    | Added build argument `CERTBOT_DNS_LOOPIA_VERSION` and installed `certbot-dns-loopia` package. |
| letsencrypt/build.yaml                    | Added argument `CERTBOT_DNS_LOOPIA_VERSION: 1.0.1` under `args`.                             |
| letsencrypt/config.yaml                   | Updated version to 5.2.4, added `loopia_username` and `loopia_password`, and included `dns-loopia` in provider list. |
| letsencrypt/rootfs/etc/cont-init.d/file-structure.sh | Added retrieval for `dns.loopia_username` and `dns.loopia_password` in configuration setup.  |
| letsencrypt/rootfs/etc/services.d/lets-encrypt/run | Integrated support for `dns-loopia`, checking for username and password, and setting propagation seconds. |

## Sequence Diagram(s)
```mermaid
sequenceDiagram
    participant User
    participant LetsEncrypt
    participant LoopiaDNS

    User->>LetsEncrypt: Request certificate
    LetsEncrypt->>LoopiaDNS: Validate domain
    LoopiaDNS-->>LetsEncrypt: Return validation result
    LetsEncrypt-->>User: Provide certificate
```
📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7b8202b and 5d925e3.

📒 Files selected for processing (2)
  • letsencrypt/CHANGELOG.md (1 hunks)
  • letsencrypt/config.yaml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • letsencrypt/CHANGELOG.md
  • letsencrypt/config.yaml

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
letsencrypt/DOCS.md (1)

573-591: Enhance the Loopia configuration documentation

Consider the following improvements to make the documentation more helpful and consistent with other provider sections:

  1. Provide a direct link to the API user creation page instead of just the customer zone
  2. List the required permissions in a more structured format
  3. Add comments in the YAML example to explain the configuration fields
 <details>
   <summary>Loopia</summary>

-To use this addon with Loopia DNS, first [create a new API user](https://customerzone.loopia.com/), with at least addZoneRecord, getZoneRecords, removeZoneRecord and removeSubdomain permissions.
+To use this addon with Loopia DNS, first [create a new API user](https://customerzone.loopia.com/api/), with the following minimum required permissions:
+
+* `addZoneRecord` - Required to create DNS records
+* `getZoneRecords` - Required to verify DNS records
+* `removeZoneRecord` - Required to clean up DNS records
+* `removeSubdomain` - Required for complete cleanup

   ```yaml
   email: [email protected]
   domains:
     - ha.yourdomain.com
   certfile: fullchain.pem
   keyfile: privkey.pem
   challenge: dns
   dns:
     provider: dns-loopia
-    loopia_username: example@loopiaapi
-    loopia_password: supersecretpasswordhere
+    loopia_username: example@loopiaapi    # Your Loopia API username
+    loopia_password: supersecretpassword  # Your Loopia API password

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used: CodeRabbit UI**
**Review profile: CHILL**

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 3c20eb004da4ec84792a20fca34c6a7cca3da7fe and 4a0411b5d6f0895b32ce121ca21811b698f15fe9.

</details>

<details>
<summary>📒 Files selected for processing (7)</summary>

* `letsencrypt/CHANGELOG.md` (1 hunks)
* `letsencrypt/DOCS.md` (4 hunks)
* `letsencrypt/Dockerfile` (2 hunks)
* `letsencrypt/build.yaml` (1 hunks)
* `letsencrypt/config.yaml` (3 hunks)
* `letsencrypt/rootfs/etc/cont-init.d/file-structure.sh` (1 hunks)
* `letsencrypt/rootfs/etc/services.d/lets-encrypt/run` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>📓 Path-based instructions (5)</summary>

<details>
<summary>letsencrypt/CHANGELOG.md (6)</summary>

Pattern `*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

---

Pattern `*/**(html|markdown|md)`: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.

---

Pattern `*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

---

Pattern `*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

---

Pattern `*/**(html|markdown|md)`: do not comment on HTML used for icons

---

Pattern `*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

</details>
<details>
<summary>letsencrypt/DOCS.md (6)</summary>

Pattern `*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

---

Pattern `*/**(html|markdown|md)`: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.

---

Pattern `*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

---

Pattern `*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

---

Pattern `*/**(html|markdown|md)`: do not comment on HTML used for icons

---

Pattern `*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

</details>
<details>
<summary>letsencrypt/Dockerfile (6)</summary>

Pattern `*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

---

Pattern `*/**(html|markdown|md)`: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.

---

Pattern `*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

---

Pattern `*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

---

Pattern `*/**(html|markdown|md)`: do not comment on HTML used for icons

---

Pattern `*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

</details>
<details>
<summary>letsencrypt/build.yaml (6)</summary>

Pattern `*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

---

Pattern `*/**(html|markdown|md)`: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.

---

Pattern `*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

---

Pattern `*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

---

Pattern `*/**(html|markdown|md)`: do not comment on HTML used for icons

---

Pattern `*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

</details>
<details>
<summary>letsencrypt/config.yaml (6)</summary>

Pattern `*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

---

Pattern `*/**(html|markdown|md)`: - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.

---

Pattern `*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"

---

Pattern `*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.

---

Pattern `*/**(html|markdown|md)`: do not comment on HTML used for icons

---

Pattern `*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.

</details>

</details>

</details>

<details>
<summary>🔇 Additional comments (14)</summary>

<details>
<summary>letsencrypt/build.yaml (2)</summary>

`28-28`: **LGTM! Version specification follows the established pattern.**

The new Loopia DNS provider version is correctly placed in alphabetical order and follows the consistent format used for other providers.

---

`28-28`: **Verify Loopia DNS plugin version compatibility.**

Let's verify that version 1.0.1 is the latest stable release and is compatible with the specified Certbot version (2.11.0).

<details>
<summary>✅ Verification successful</summary>

**Version 1.0.1 is compatible with Certbot 2.11.0**

The verification confirms that:
- 1.0.1 is indeed the latest version of certbot-dns-loopia on PyPI
- It requires certbot >= 1.7.0, which makes it compatible with Certbot 2.11.0
- All other dependencies (acme >= 1.8.0, loopialib >= 0.2.0) are at compatible versions



</details>

<details>
<summary>🏁 Scripts executed</summary>

The following scripts were executed for the analysis:

Script:

```shell
#!/bin/bash
# Description: Verify the latest version of certbot-dns-loopia and its compatibility

# Test: Check if 1.0.1 is the latest version on PyPI
curl -s "https://pypi.org/pypi/certbot-dns-loopia/json" | jq -r '.info.version'

# Test: Check package dependencies to verify Certbot version compatibility
curl -s "https://pypi.org/pypi/certbot-dns-loopia/1.0.1/json" | jq -r '.info.requires_dist[]'

Length of output: 312

letsencrypt/config.yaml (3)

78-79: LGTM! Loopia credentials follow the established pattern.

The new configuration fields for Loopia credentials:

  • Follow the same pattern as other DNS providers
  • Are correctly marked as optional
  • Are properly placed in alphabetical order

108-108: LGTM! Provider list updated correctly.

The dns-loopia provider:

  • Follows the naming convention
  • Is properly placed in alphabetical order
  • Maintains the existing format

2-2: 🛠️ Refactor suggestion

Consider bumping minor version instead of patch.

Since adding support for a new DNS provider is a feature enhancement rather than a bug fix, consider bumping the minor version (5.3.0) instead of the patch version (5.2.4).

letsencrypt/CHANGELOG.md (1)

5-5: LGTM!

The changelog entry follows the established format and clearly describes the added feature, consistent with previous DNS provider additions in the changelog history.

letsencrypt/Dockerfile (2)

74-74: LGTM! Package installation follows the established pattern.

The addition of certbot-dns-loopia package follows the existing pattern, using the version variable consistently and maintaining alphabetical ordering.

#!/bin/bash
# Verify the package version matches the one mentioned in PR objectives
rg "CERTBOT_DNS_LOOPIA_VERSION: 1.0.1" build.yaml

24-24: LGTM! Build argument follows the established pattern.

The addition of CERTBOT_DNS_LOOPIA_VERSION follows the existing pattern and is correctly positioned alphabetically among other DNS provider version arguments.

✅ Verification successful

✓ Build argument is correctly defined and consistent

The CERTBOT_DNS_LOOPIA_VERSION build argument is properly defined in letsencrypt/build.yaml with version 1.0.1, maintaining consistency between the Dockerfile and build configuration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the version is properly defined in build.yaml
rg "CERTBOT_DNS_LOOPIA_VERSION:" build.yaml

Length of output: 102


Script:

#!/bin/bash
# Let's check for build configuration files in different locations
fd -t f "build.ya?ml"

# Also check for any yaml files that might contain version definitions
fd -t f -e yaml | xargs rg "CERTBOT_DNS_LOOPIA_VERSION"

# Check if there are any other files defining build arguments
rg -g "!node_modules" "ARG CERTBOT_DNS_LOOPIA_VERSION"

Length of output: 772

letsencrypt/rootfs/etc/cont-init.d/file-structure.sh (1)

36-37: LGTM! The Loopia DNS configuration is properly integrated.

The addition of Loopia DNS credentials follows the established pattern and security practices:

  • Configuration fields match the certbot-dns-loopia plugin requirements
  • Credentials are stored in the protected dnsapikey file (chmod 600)
  • Lines are added in alphabetical order maintaining code organization

Let's verify the configuration field names match the plugin requirements:

✅ Verification successful

The Loopia DNS configuration is properly integrated and verified

The verification confirms that:

  1. The configuration fields dns.loopia_username and dns.loopia_password are correctly defined in config.yaml schema
  2. The Loopia plugin certbot-dns-loopia is properly installed via Dockerfile
  3. The credentials are correctly handled in the service run script with appropriate propagation time (minimum 900 seconds due to caching issues)
  4. The implementation follows the same secure pattern as other DNS providers:
    • Credentials stored in protected dnsapikey file
    • Proper authentication arguments passed to certbot

The review comment's approval was correct and the implementation is verified to be secure and consistent with the codebase's patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Loopia DNS plugin configuration field names
# Expected: The configuration fields should match those required by certbot-dns-loopia

# Test: Check if the plugin is listed in requirements.txt
rg -l "certbot-dns-loopia" requirements.txt

# Test: Check configuration field documentation
rg "dns.loopia_(username|password)" "letsencrypt/config.yaml"

Length of output: 162


Script:

#!/bin/bash
# Let's try a broader search to find relevant files and requirements

# Find requirements or dependency files
fd -t f "requirements.*|setup.*|package.*"

# Search for Loopia-related configuration in yaml files
fd -e yaml -e yml -x rg -l "loopia"

# Search for Loopia plugin references in the entire codebase
rg -l "loopia" -g "!*.md"

# Search for DNS provider patterns in config.yaml to verify structure
fd "config.yaml" -x rg "dns\." {}

Length of output: 319


Script:

#!/bin/bash
# Let's examine the specific files we found for Loopia configuration

# Check config.yaml for Loopia DNS configuration structure
cat letsencrypt/config.yaml

# Check Dockerfile for plugin installation
cat letsencrypt/Dockerfile

# Check the service run script for Loopia handling
cat "letsencrypt/rootfs/etc/services.d/lets-encrypt/run"

Length of output: 25588

letsencrypt/rootfs/etc/services.d/lets-encrypt/run (2)

175-175: Consider standardizing provider argument patterns across all providers.

While the implementation is correct, there's an inconsistency in how provider arguments are structured across different providers. Some use direct flags (e.g., --dns-cloudflare), while others use --authenticator. Consider standardizing this in a future update.

Let's analyze the current patterns:

#!/bin/bash
# Description: Analyze provider argument patterns
# Expected: Show different patterns used across providers

echo "Analyzing provider argument patterns..."
rg -B 1 'PROVIDER_ARGUMENTS\+=' | grep -v 'PROVIDER_ARGUMENTS+='

167-176: LGTM! Implementation follows established patterns.

The Loopia DNS provider implementation correctly:

  • Requires necessary credentials
  • Handles DNS propagation time with appropriate minimum value
  • Uses consistent provider arguments pattern

Let's verify the configuration schema matches the implementation:

✅ Verification successful

Configuration schema correctly includes Loopia DNS provider requirements

The implementation matches the configuration schema in letsencrypt/config.yaml:

  • Required fields loopia_username and loopia_password are properly defined as optional string fields in the schema
  • The DNS provider dns-loopia is correctly listed in the provider schema options
  • The implementation aligns with the schema definition
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that config.yaml includes the required Loopia DNS fields
# Expected: config.yaml should contain loopia_username and loopia_password fields

echo "Checking config.yaml for Loopia DNS configuration..."
rg -A 2 'loopia_username:|loopia_password:' config.yaml || echo "Required Loopia configuration fields not found in config.yaml"

Length of output: 3480


Script:

#!/bin/bash
# Let's check the config schema in the repository
echo "Searching for config schema files..."
fd -e json -e yaml schema

Length of output: 110


Script:

#!/bin/bash
# Let's try a different approach to find configuration schema
echo "Searching for configuration files..."
fd config.json config.yaml

Length of output: 210


Script:

#!/bin/bash
# Let's check the repository root for configuration files
echo "Listing files in repository root..."
ls -la

# Also search for any YAML files that might contain configuration
echo -e "\nSearching for YAML files..."
fd -e yaml -e yml

Length of output: 4300


Script:

#!/bin/bash
# Let's check the letsencrypt config.yaml for DNS configuration schema
echo "Checking letsencrypt/config.yaml content..."
cat letsencrypt/config.yaml

Length of output: 3666

letsencrypt/DOCS.md (3)

62-62: LGTM! Provider listing is consistent

The DNS provider 'dns-loopia' has been correctly added to both provider lists while maintaining alphabetical ordering.

Also applies to: 1101-1101


122-123: LGTM! Configuration fields are properly defined

The Loopia configuration fields follow the established naming convention and are correctly placed in alphabetical order.


Line range hint 573-1101: LGTM! Documentation maintains quality and consistency

The documentation changes maintain the established structure and formatting conventions of the project.

Comment on lines +3 to +4
## 5.2.4

Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider bumping minor version instead of patch.

Adding support for a new DNS provider is a feature addition rather than a bug fix. Based on semantic versioning principles and the pattern seen in previous DNS provider additions (e.g., IONOS DNS in 5.2.2), this should be version 5.3.0 instead of 5.2.4.

-## 5.2.4
+## 5.3.0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 5.2.4
## 5.3.0

Copy link
Member

Choose a reason for hiding this comment

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

This add-on used patch version to add new DNS providers, so let's stay consistent and continue with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@agners Understood, we'll maintain consistency with the existing versioning scheme.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
letsencrypt/DOCS.md (1)

573-599: Standardize list marker style in Loopia documentation.

The required permissions list uses asterisks (*) while other lists in the document use dashes (-). Maintain consistency with the document's style.

Apply this change to the permissions list:

-* `addZoneRecord` - Required to create DNS records
-* `getZoneRecords` - Required to verify DNS records
-* `removeZoneRecord` - Required to clean up DNS records
-* `removeSubdomain` - Required for complete cleanup
+- `addZoneRecord` - Required to create DNS records
+- `getZoneRecords` - Required to verify DNS records
+- `removeZoneRecord` - Required to clean up DNS records
+- `removeSubdomain` - Required for complete cleanup
🧰 Tools
🪛 Markdownlint

578-578: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


579-579: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


580-580: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


581-581: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 44c6034 and 9e68ed8.

📒 Files selected for processing (1)
  • letsencrypt/DOCS.md (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
letsencrypt/DOCS.md (6)

Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.

  • Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
  • In step-by-step instructions, front the location phrase in the instructional sentence.
  • In step-by-step instructions, front the 'goal' in the instructional sentence.
  • In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
  • do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'

Pattern */**(html|markdown|md): - Use bold to mark UI strings.

  • If "" are used to mark UI strings, replace them by bold.

Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"


Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.


Pattern */**(html|markdown|md): do not comment on HTML used for icons


Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.

🪛 Markdownlint
letsencrypt/DOCS.md

578-578: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


579-579: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


580-580: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)


581-581: Expected: dash; Actual: asterisk
Unordered list style

(MD004, ul-style)

🔇 Additional comments (3)
letsencrypt/DOCS.md (3)

62-62: LGTM: Provider listing is consistent.

The Loopia DNS provider has been correctly added to both provider listings in the document.

Also applies to: 1108-1108


122-123: LGTM: Configuration fields are properly documented.

The Loopia credentials fields are correctly added to the configuration section with appropriate placeholders.


585-596: LGTM: Configuration example is clear and well-structured.

The YAML configuration example follows the established pattern and includes all necessary fields.

Adhere to coderabbits suggestion
@agnon
Copy link
Contributor Author

agnon commented Nov 10, 2024

@coderabbitai review

@agnon
Copy link
Contributor Author

agnon commented Nov 16, 2024

I see the builds got rate limited, or at least it looks like that. Is someone looking into this?

Copy link
Member

@sairon sairon 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 to me. The certbot-dns-loopia hasn't been touched for a while but it seems the maintainer is still around, so hopefully he'll be cooperative if there's need for an update.

@agners agners merged commit 35b68b3 into home-assistant:master Nov 25, 2024
11 checks passed
@moorgrove
Copy link

moorgrove commented Nov 25, 2024

I get errors below when i try to update with Loopia DNS challenge. Any idea what it could be?

Missing property in credentials configuration file /data/dnsapikey:
 * Property "dns_loopia_user" not found (should be API username for Loopia account).

Settings:

domains:
  - xxx
email: xxx
keyfile: xxx
certfile: xxx
challenge: dns
dns:
  provider: dns-loopia
  loopia_username: xxx
  loopia_password: xxx

@agnon agnon deleted the dns-loopia-support branch November 26, 2024 01:21
@agnon
Copy link
Contributor Author

agnon commented Nov 26, 2024

Sorry, got the username wrong in the dnsapikey file. New PR #3838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants