Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new debugging output and emojis in
ci/aws.sh(e.g., printing AWS_REGION/AWS_BUCKET and the TOML checks) look like temporary diagnostics; consider gating them behind a debug flag or removing them before merging to keep CI logs clean. - By dropping the
STS_ENDPOINTlogic inci/aws.shand the EUSC-specificendpointsoverrides inaws_config_builder.py, the PR removes the special handling foreusc-*regions; if those regions are still in scope, consider retaining or replacing this behavior to avoid region-specific DNS/endpoint issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new debugging output and emojis in `ci/aws.sh` (e.g., printing AWS_REGION/AWS_BUCKET and the TOML checks) look like temporary diagnostics; consider gating them behind a debug flag or removing them before merging to keep CI logs clean.
- By dropping the `STS_ENDPOINT` logic in `ci/aws.sh` and the EUSC-specific `endpoints` overrides in `aws_config_builder.py`, the PR removes the special handling for `eusc-*` regions; if those regions are still in scope, consider retaining or replacing this behavior to avoid region-specific DNS/endpoint issues.
## Individual Comments
### Comment 1
<location> `cloud/opentofu/aws_config_builder.py:28-37` </location>
<code_context>
'skip_region_validation': True
}
- # Map .eu endpoints for EUSC regions to prevent DNS "no such host" errors
- if region.startswith('eusc-'):
- # These are the primary services used by this builder
- services = ['ec2', 'sts', 'iam']
- provider_config['endpoints'] = []
- for service in services:
- provider_config['endpoints'].append({
- service: f'https://{service}.{region}.amazonaws.eu'
- })
-
- return provider_config
-
def build_resources(self):
</code_context>
<issue_to_address>
**question (bug_risk):** Removing EUSC-specific endpoint mapping may reintroduce DNS issues for those regions.
This change removes the explicit mapping of `eusc-*` regions to `.amazonaws.eu` endpoints that previously avoided DNS "no such host" errors for EC2/STS/IAM. Please confirm that the default AWS endpoint resolution now correctly supports EUSC (and any similar) regions, or consider keeping a minimal override for those regions if they’re still in the supported matrix.
</issue_to_address>
### Comment 2
<location> `schutzbot/deploy.sh:122` </location>
<code_context>
else
# Capture response and HTTP code to handle GitHub API failures (e.g. 401, 403)
- response=$(curl -u "${API_USER}:${API_PAT}" -s -w "%{http_code}" "https://api.github.com/repos/osbuild/osbuild-composer/commits?per_page=100")
+ response=$(curl -u "${API_USER}:${API_PAT}" -s -w "%{http_code}" "https://api.github.com/repos/osbuild/osbuild-composer/commits?per_page=10")
http_code="${response: -3}"
body="${response::-3}"
</code_context>
<issue_to_address>
**suggestion:** Reducing `per_page` from 100 to 10 might miss the last passed commit in busy repos.
With only 10 commits, any last passing commit older than those 10 (e.g., after several failing builds or high commit volume) won’t be found, which may change downstream behavior. If this is about reducing payload size, consider a higher limit (e.g., 50–100) or add pagination instead of dropping to 10.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Map .eu endpoints for EUSC regions to prevent DNS "no such host" errors | ||
| if region.startswith('eusc-'): | ||
| # These are the primary services used by this builder | ||
| services = ['ec2', 'sts', 'iam'] | ||
| provider_config['endpoints'] = [] | ||
| for service in services: | ||
| provider_config['endpoints'].append({ | ||
| service: f'https://{service}.{region}.amazonaws.eu' | ||
| }) | ||
|
|
There was a problem hiding this comment.
question (bug_risk): Removing EUSC-specific endpoint mapping may reintroduce DNS issues for those regions.
This change removes the explicit mapping of eusc-* regions to .amazonaws.eu endpoints that previously avoided DNS "no such host" errors for EC2/STS/IAM. Please confirm that the default AWS endpoint resolution now correctly supports EUSC (and any similar) regions, or consider keeping a minimal override for those regions if they’re still in the supported matrix.
| else | ||
| # Capture response and HTTP code to handle GitHub API failures (e.g. 401, 403) | ||
| response=$(curl -u "${API_USER}:${API_PAT}" -s -w "%{http_code}" "https://api.github.com/repos/osbuild/osbuild-composer/commits?per_page=100") | ||
| response=$(curl -u "${API_USER}:${API_PAT}" -s -w "%{http_code}" "https://api.github.com/repos/osbuild/osbuild-composer/commits?per_page=10") |
There was a problem hiding this comment.
suggestion: Reducing per_page from 100 to 10 might miss the last passed commit in busy repos.
With only 10 commits, any last passing commit older than those 10 (e.g., after several failing builds or high commit volume) won’t be found, which may change downstream behavior. If this is about reducing payload size, consider a higher limit (e.g., 50–100) or add pagination instead of dropping to 10.
No description provided.