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

Infra cost reduction (Move from ECS to EC2) #387

Merged
merged 7 commits into from
Oct 18, 2024
Merged

Conversation

LDMGN
Copy link
Contributor

@LDMGN LDMGN commented Sep 10, 2024

No description provided.

@LDMGN LDMGN self-assigned this Sep 10, 2024
@LDMGN LDMGN changed the title Infra cost reduction Infra cost reduction (Move from ECS to EC2) Sep 10, 2024
Copy link

Overall Project 73.64% 🍏

There is no coverage information present for the Files changed

Instance,
InstanceClass,
InstanceSize,
InstanceType, Peer, Port,
InstanceType, KeyPair,
Copy link
Contributor

Choose a reason for hiding this comment

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

single import on one line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, and removed one unused import.

/* Database access. */
const databaseSecurityGroup = SecurityGroup.fromSecurityGroupId(this, 'DatabaseSecurityGroup', props.database.securityGroup.id);
databaseSecurityGroup.connections.allowFrom(ec2SecurityGroup, Port.tcp(parseInt(props.database.port)));
// databaseSecurityGroup.connections.allowFrom(ec2SecurityGroup, Port.tcp(parseInt(props.database.port)));
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep this as comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no. It should be as limited as possible, but I don't have time to test it right now. So for now at least I'll add a TODO.

import {
Credentials,
DatabaseInstance,
DatabaseInstanceEngine,
DatabaseInstanceEngine, DatabaseInstanceFromSnapshot,
Copy link
Contributor

Choose a reason for hiding this comment

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

one import on single line ?
Check line 9 as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by auto-formatting

Copy link
Contributor

@MarcelWildenburg MarcelWildenburg left a comment

Choose a reason for hiding this comment

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

some minor comments on formatting and unused import.

Copy link

Overall Project 72.57% 🍏

There is no coverage information present for the Files changed

@LDMGN LDMGN merged commit ab058b2 into main Oct 18, 2024
4 checks passed
@LDMGN LDMGN deleted the infra-cost-reduction branch October 18, 2024 20:31
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