Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IT-4243: fix region lookup #21

Merged
merged 6 commits into from
Jan 30, 2025
Merged

Conversation

hallieswan
Copy link
Contributor

Description

PR #19 included a global var for the AWS region that looked up the value from aws_cdk.App().region. However, this returns None, so the workflow job failed. This PR will instead get the region value from the workflow input.

@hallieswan hallieswan requested review from a team as code owners January 29, 2025 16:47
app.py Show resolved Hide resolved
app.py Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
.github/workflows/aws-deploy.yaml Outdated Show resolved Hide resolved
@hallieswan hallieswan requested a review from zaro0508 January 29, 2025 19:27
src/bastion_props.py Show resolved Hide resolved
src/service_stack.py Outdated Show resolved Hide resolved
app.py Outdated
Comment on lines 181 to 182
certificate_account_id=environment_variables["CERTIFICATE_ACCOUNT_ID"],
certificate_resource_id=environment_variables["CERTIFICATE_RESOURCE_ID"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that this solution requires setting tags because it's kinda a jenky work around.

app.py Show resolved Hide resolved
@hallieswan hallieswan requested a review from zaro0508 January 29, 2025 23:22
app.py Outdated Show resolved Hide resolved
# https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_ec2/Instance.html
self.instance = ec2.Instance(
self,
"BastionHost",
instance_type=props.instance_type,
machine_image=ec2.MachineImage.generic_linux(
{props.ami_region: props.ami_id}
{region_json.to_string(): props.ami_id}
Copy link
Contributor

Choose a reason for hiding this comment

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

if you can reference with self.region in LoadBalancedServiceStack then you can probably do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using self.region as the key in a map causes this error -- RuntimeError: Error: "${Token[AWS.Region.11]}" is used as the key in a map so must resolve to a string, but it resolves to: {"Ref":"AWS::Region"}. Consider using "CfnJson" to delay resolution to deployment-time.

Copy link
Contributor

Choose a reason for hiding this comment

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

wacko! i'm not a fan of CfnJson workaround but it works so let's just move on for now.

app.py Outdated Show resolved Hide resolved
@hallieswan hallieswan requested a review from zaro0508 January 30, 2025 17:11
# https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_ec2/Instance.html
self.instance = ec2.Instance(
self,
"BastionHost",
instance_type=props.instance_type,
machine_image=ec2.MachineImage.generic_linux(
{props.ami_region: props.ami_id}
{region_json.to_string(): props.ami_id}
Copy link
Contributor

Choose a reason for hiding this comment

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

wacko! i'm not a fan of CfnJson workaround but it works so let's just move on for now.

@hallieswan hallieswan merged commit 3ff13d2 into Sage-Bionetworks-IT:dev Jan 30, 2025
4 checks passed
@hallieswan hallieswan deleted the IT-4243 branch January 30, 2025 18:00
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.

2 participants