Skip to content

Conversation

balamurali27
Copy link
Contributor

@balamurali27 balamurali27 commented Jul 17, 2025

What copilot said

@balamurali27 balamurali27 requested a review from Copilot July 17, 2025 07:43
Copilot

This comment was marked as outdated.

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

❌ Patch coverage is 65.78947% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 27.35%. Comparing base (0cad17b) to head (6c4fe1e).
⚠️ Report is 931 commits behind head on develop.

Files with missing lines Patch % Lines
press/press/doctype/root_domain/root_domain.py 65.78% 13 Missing ⚠️

❌ Your patch check has failed because the patch coverage (65.78%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2806      +/-   ##
===========================================
- Coverage    36.34%   27.35%   -8.99%     
===========================================
  Files          431      431              
  Lines        35589    35684      +95     
===========================================
- Hits         12934     9761    -3173     
- Misses       22655    25923    +3268     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Guessing is risky

- Also create dns record for default proxy server set
- Add test for the same
- Update proxy server tests to not have IP as it is fetched from virtual_machine
@balamurali27 balamurali27 requested a review from adityahase as a code owner July 18, 2025 10:12
@balamurali27 balamurali27 requested a review from Copilot July 18, 2025 10:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements automatic hosted zone creation on AWS Route 53 for root domains, with enhanced DNS record management capabilities.

  • Creates AWS hosted zones automatically when root domains are created
  • Adds NS record management for subdomain delegation between root domains
  • Refactors test infrastructure to support mocked AWS operations

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
root_domain.json Adds new fields for parent domain linking and hosted zone tracking
root_domain.py Implements hosted zone creation, NS record management, and wildcard DNS record functionality
test_root_domain.py Adds comprehensive tests for AWS hosted zone creation and DNS record management
test_proxy_server.py Removes hardcoded IP addresses to support dynamic IP assignment
test_virtual_machine.py Adds public IP address parameter for better test consistency

Comment on lines +162 to +175
# If the hosted zone already exists, we can safely ignore this error
if not self.hosted_zone:
zone = self.boto3_client.create_hosted_zone(
Name=self.name,
CallerReference=str(datetime.now()),
HostedZoneConfig={
"Comment": f"Press Root Domain for {self.default_cluster}",
"PrivateZone": False,
},
)
self.hosted_zone = zone["HostedZone"]["Id"]
else:
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
self.add_hosted_zone_ns(zone["DelegationSet"]["NameServers"])
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

This comment is misplaced and doesn't correspond to any error handling logic. The comment suggests error handling that isn't present in the code below it.

Suggested change
# If the hosted zone already exists, we can safely ignore this error
if not self.hosted_zone:
zone = self.boto3_client.create_hosted_zone(
Name=self.name,
CallerReference=str(datetime.now()),
HostedZoneConfig={
"Comment": f"Press Root Domain for {self.default_cluster}",
"PrivateZone": False,
},
)
self.hosted_zone = zone["HostedZone"]["Id"]
else:
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
self.add_hosted_zone_ns(zone["DelegationSet"]["NameServers"])
try:
if not self.hosted_zone:
zone = self.boto3_client.create_hosted_zone(
Name=self.name,
CallerReference=str(datetime.now()),
HostedZoneConfig={
"Comment": f"Press Root Domain for {self.default_cluster}",
"PrivateZone": False,
},
)
self.hosted_zone = zone["HostedZone"]["Id"]
else:
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
self.add_hosted_zone_ns(zone["DelegationSet"]["NameServers"])
except self.boto3_client.exceptions.HostedZoneAlreadyExistsException:
log_error("Hosted zone already exists. Skipping creation.", domain=self.name)

Copilot uses AI. Check for mistakes.

}
]
},
HostedZoneId=parent_zone,
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

The parent_zone variable contains the full hosted zone ID path, but this line expects only the zone ID. It should be parent_zone.split('/')[-1] to extract just the ID portion, similar to line 88.

Suggested change
HostedZoneId=parent_zone,
HostedZoneId=parent_zone.split("/")[-1],

Copilot uses AI. Check for mistakes.

)
self.hosted_zone = zone["HostedZone"]["Id"]
else:
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

When self.hosted_zone already exists (else branch), the zone variable from get_hosted_zone response doesn't contain a DelegationSet key. This will cause a KeyError. Use zone['HostedZone']['DelegationSet']['NameServers'] instead.

Suggested change
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
zone = self.boto3_client.get_hosted_zone(Id=self.hosted_zone.split("/")[-1])
if "DelegationSet" not in zone or "NameServers" not in zone["DelegationSet"]:
log_error("DelegationSet or NameServers missing in hosted zone response", domain=self.name)
return

Copilot uses AI. Check for mistakes.

if not self.hosted_zone:
zone = self.boto3_client.create_hosted_zone(
Name=self.name,
CallerReference=str(datetime.now()),
Copy link

Copilot AI Jul 18, 2025

Choose a reason for hiding this comment

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

Missing import for datetime. Add from datetime import datetime at the top of the file.

Copilot uses AI. Check for mistakes.

@adityahase adityahase removed their request for review September 18, 2025 09:09
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.

1 participant