fix(dashmate): zero ssl certificate draft not saved #2297
Conversation
WalkthroughThis pull request introduces two new parameters, Changes
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/dashmate/src/ssl/zerossl/validateZeroSslCertificateFactory.js (1)
116-118: Clarify the certificate state comments.The comment "Certificate is already created" might be misleading here since the subsequent code checks for
status !== 'issued'. This could cause confusion about the actual state of the certificate.Consider updating the comments to be more precise:
- // Certificate is already created at this point, so we just need to pass validation - // and download certificate file + // Certificate record exists at this point, but we need to verify its status + // and potentially download the certificate file if validation is completepackages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (2)
Line range hint
173-201: Consider adding a maximum retry duration for verificationThe current implementation has a retry prompt without a maximum duration limit. This could potentially lead to extended waiting times if there are persistent network or firewall issues.
Consider adding a timeout mechanism:
task: async (ctx, task) => { let retry; + const startTime = Date.now(); + const MAX_RETRY_DURATION = 30 * 60 * 1000; // 30 minutes do { try { await verifyDomain(ctx.certificate.id, ctx.apiKey); } catch (e) { + if (Date.now() - startTime > MAX_RETRY_DURATION) { + throw new Error('Certificate verification timed out after 30 minutes. Please verify your network configuration and try again.'); + } if (ctx.noRetry !== true) { retry = await task.prompt({
Line range hint
211-235: Consider making certificate download retry parameters configurableThe current implementation uses hardcoded values for retry count (50) and interval (5000ms). These could be made configurable to accommodate different network conditions or API response times.
Consider extracting these as configurable parameters:
+const DEFAULT_DOWNLOAD_RETRY_COUNT = 50; +const DEFAULT_DOWNLOAD_RETRY_INTERVAL = 5000; export default function obtainZeroSSLCertificateTaskFactory( generateCsr, generateKeyPair, createZeroSSLCertificate, verifyDomain, downloadCertificate, getCertificate, listCertificates, saveCertificateTask, verificationServer, homeDir, validateZeroSslCertificate, configFileRepository, configFile, + downloadRetryCount = DEFAULT_DOWNLOAD_RETRY_COUNT, + downloadRetryInterval = DEFAULT_DOWNLOAD_RETRY_INTERVAL, ) { // ... existing code ... task: async (ctx, task) => { - for (let retry = 0; retry <= 50; retry += 1) { - await wait(5000); + for (let retry = 0; retry <= downloadRetryCount; retry += 1) { + await wait(downloadRetryInterval);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js(3 hunks)packages/dashmate/src/ssl/zerossl/validateZeroSslCertificateFactory.js(2 hunks)
🔇 Additional comments (3)
packages/dashmate/src/ssl/zerossl/validateZeroSslCertificateFactory.js (2)
Line range hint 107-114: LGTM! Clear error handling for pending and draft certificates.
The logic correctly identifies certificates in 'pending_validation' or 'draft' states and sets appropriate flags for further processing.
Line range hint 107-118: Verify integration with configuration saving mechanism.
While the validation logic is correct, let's verify that this validation flow properly integrates with the new configuration saving mechanism mentioned in the PR objectives.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the certificate validation and configuration saving mechanisms:
Certificate validation is properly integrated with configuration saving
The code review shows that the validation flow is correctly integrated with the configuration saving mechanism through:
- The
saveCertificateTaskwhich handles certificate persistence - The
obtainZeroSSLCertificateTaskFactorywhich coordinates between validation and saving - The
configFileRepositorydependency injection in the certificate handling flow
The validation result in validateZeroSslCertificateFactory.js is properly consumed by these components to ensure certificates are only saved when valid, preventing the infinite loop scenario mentioned in the PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related configuration saving logic in the codebase
echo "Searching for configuration saving mechanism..."
rg -l "configFileRepository.*save.*certificate"
# Search for potential infinite loop prevention
echo "Searching for draft certificate handling..."
rg "zerossl.*draft" --type js
Length of output: 283
Script:
#!/bin/bash
# Let's try a broader search for configuration saving patterns
echo "Searching for certificate configuration patterns..."
rg -A 5 "saveCertificate|saveConfig.*certificate" --type js
# Search for ZeroSSL certificate handling
echo "Searching for ZeroSSL certificate handling..."
rg -A 5 "ZeroSslCertificate" --type js
# Look for files related to certificate configuration
echo "Finding certificate configuration files..."
fd -e js certificate
Length of output: 32885
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js (1)
21-22: LGTM: Parameter additions are well-documented
The new parameters configFileRepository and configFile are properly added and documented in JSDoc, enabling the configuration persistence functionality needed to fix the draft certificate issue.
Also applies to: 37-38
packages/dashmate/src/listr/tasks/ssl/zerossl/obtainZeroSSLCertificateTaskFactory.js
Show resolved
Hide resolved
Issue being fixed or feature implemented
In case of error during ZeroSSL certificate validation, a draft certificate what we just created is not saving. Together with dashmate helper it creates an infinite loop of drafts spamming ZeroSSL API.
What was done?
How Has This Been Tested?
Running locally
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes