Skip to content

Conversation

@sicarul
Copy link
Contributor

@sicarul sicarul commented Oct 16, 2025

I tried using Pulumi import but it'd not get the properties and then it'd fail - Using lookup pulumi is able to reference the resource anyway without bringing it into state.

@claude
Copy link
Contributor

claude bot commented Oct 16, 2025

PR Review: Add CDN Log Delivery source

Issues Found

1. Capitalization inconsistency (line 803)
The comment uses "Cdn" which should be "CDN" (an acronym for Content Delivery Network). According to the style guide, acronyms should be fully capitalized.

2. Incomplete migration
The change switches from creating a new LogDeliverySource to referencing an existing one via .get(), but there are concerns:

  • The hardcoded ID "CreatedByCloudFront-E3PRSXO1BZJEEY" appears to be environment-specific and may not work across different deployments
  • The resource name "cloudfront_logs" doesn't follow the naming pattern used elsewhere in the file (which uses kebab-case like "cdn-log-delivery-destination")
  • Line 824 references cdnLogDeliverySource.name, but the .get() method's second parameter ("cloudfront_logs") is the resource name in Pulumi state, not the actual AWS resource name. This may cause runtime issues if the AWS resource has a different name.

3. Code verification needed
Before merging, verify that:

  • The cdnLogDeliverySource.name property returns the correct AWS resource name expected by the cdnLogDelivery resource on line 824
  • This configuration has been tested in the target environment
  • The hardcoded ID works in production

Suggested Changes

Line 803:

-// The Cdn Log Delivery Source was configured outside of Pulumi
+// The CDN Log Delivery Source was configured outside of Pulumi

Lines 806-809:
Consider adding a comment explaining where these values come from:

// Reference the CloudFront-created log delivery source
// ID: CreatedByCloudFront-E3PRSXO1BZJEEY (created automatically by CloudFront)
// Resource name in Pulumi state: cloudfront_logs
const cdnLogDeliverySource = aws.cloudwatch.LogDeliverySource.get(
    "CreatedByCloudFront-E3PRSXO1BZJEEY",
    "cloudfront_logs"
);

Questions

  1. Have you verified that cdnLogDeliverySource.name returns the correct AWS resource name for use in the LogDelivery resource?
  2. Is the hardcoded ID "CreatedByCloudFront-E3PRSXO1BZJEEY" stable across environments, or should this be parameterized?
  3. Has this change been tested to ensure the log delivery continues to work correctly?

@pulumi-bot
Copy link
Collaborator

@sicarul sicarul enabled auto-merge (squash) October 16, 2025 18:29
@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@sicarul sicarul merged commit 8a19a42 into master Oct 16, 2025
8 checks passed
@sicarul sicarul deleted the import-existing-delivery-source branch October 16, 2025 18:37
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.

4 participants