Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR enables Azure Container Apps to issue and bind TLS certificates directly via Azure managed-certificate ARM APIs. It introduces a ChangesAzure BYOC Certificate Issuance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/cli/cert.go`:
- Around line 117-124: GenerateLetsEncryptCert currently logs issuer.IssueCert
errors via term.Errorf but always returns nil, so callers can't detect failures;
change GenerateLetsEncryptCert to collect errors from each issuer.IssueCert call
(e.g., append to a slice or use an error-aggregator) and after the loop return a
consolidated non-nil error when any IssueCert failed (or wrap multiple errors
with fmt.Errorf/%w or a multierror), preserving the existing term.Errorf logging
but propagating the error out of GenerateLetsEncryptCert so callers can react to
issuance failures.
In `@src/pkg/cli/client/byoc/azure/cert.go`:
- Around line 353-362: The managedCertName function can produce a trailing
hyphen when you do sanitize() then slice (env[:15]/host[:30]); fix by ensuring
you remove any trailing hyphens after truncation. Specifically, in
managedCertName sanitize envName and hostname, then apply the length limits,
then trim any leading or trailing hyphens from the truncated strings (and handle
the case of an empty result by providing a fallback like "env" or "host");
update the code around the managedCertName function to perform the
post-truncation hyphen trim so the returned fmt.Sprintf("mc-%s-%s", env, host)
never contains consecutive hyphens.
- Around line 157-182: The PATCH body in addHostnameDisabled and
bindHostnameSniEnabled replaces the entire CustomDomains array, which drops
existing registrations; instead, ensure you preserve and update the
current.ContainerApp.Properties.Configuration.Ingress.CustomDomains: in
addHostnameDisabled use the provided current to prepend/append the new
armappcontainers.CustomDomain (checking hasCustomDomain first) to
current.Properties.Configuration.Ingress.CustomDomains and pass that full array
in the BeginUpdate body; in bindHostnameSniEnabled, fetch the current app if not
provided, locate and update the matching CustomDomain entry in
current.Properties.Configuration.Ingress.CustomDomains (set
SniEnabled/CertificateId/etc.) or append if missing, then call BeginUpdate with
the complete modified CustomDomains array so other domains are not overwritten.
In `@src/pkg/dns/check.go`:
- Around line 94-101: The function LookupTXTContains currently logs with
term.Debugf which bypasses the injectable package Logger; update the logging
call inside LookupTXTContains to use the package-level Logger (Logger.Debugf) so
tests can override or silence logs, keeping the existing message and arguments
(name, txts, err) and leaving the rest of the function unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4977f295-93cb-462d-9bfb-098206156d2c
📒 Files selected for processing (8)
src/pkg/cli/cert.gosrc/pkg/cli/cert_test.gosrc/pkg/cli/client/byoc/aws/domain_test.gosrc/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/azure/cert.gosrc/pkg/dns/check.gosrc/pkg/dns/mock.gosrc/pkg/dns/resolver.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/cli/cert.go`:
- Line 140: The current return returns errors.Join(issueErrs...) which exposes
raw errors without remediation; update the caller in the cert issuance function
(where issueErrs is assembled and returned) to wrap the joined error with a
user-facing message that includes clear remediation steps (e.g., "failed to
issue certificate: <joined errors>; try checking X, Y, or run `command`") by
constructing a new error (using fmt.Errorf or errors.Join+fmt.Errorf) that
prefixes a short actionable instruction and includes the joined issueErrs for
diagnostics; ensure the change references the issueErrs variable and the
function that returns it so logs and callers receive the wrapped, actionable
error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 683b0d06-05ff-4fee-ab90-f5542b64a54a
📒 Files selected for processing (4)
src/pkg/cli/cert.gosrc/pkg/cli/client/byoc/azure/cert.gosrc/pkg/cli/client/byoc/azure/cert_test.gosrc/pkg/dns/check.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pkg/cli/client/byoc/azure/cert_test.go
- src/pkg/cli/client/byoc/azure/cert.go
2 participants
Description
Implement azure byod cert gen
Linked Issues
DefangLabs/pulumi-defang#216
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests