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

HL-1205 Postgres support #80

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

masiorski
Copy link

@masiorski masiorski commented Feb 4, 2020

Please review and release this PR. I'm leaving atlassian in February. I would like to finish PG support before my leave. Regards, Marcin

CHANGELOG.md Outdated
[Unreleased]: https://github.com/atlassian/aws-infrastructure/compare/release-2.20.0...master
[Unreleased]: https://github.com/atlassian/aws-infrastructure/compare/release-2.20.2...master

## [2.20.2] - 2020-02-04
Copy link
Contributor

Choose a reason for hiding this comment

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

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and

https://semver.org/

Shouldn't it be 2.21.0?

Copy link
Author

Choose a reason for hiding this comment

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

it can be sir :) I will change this

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the header, we'll add it after release

CHANGELOG.md Outdated
[Unreleased]: https://github.com/atlassian/aws-infrastructure/compare/release-2.20.0...master
[Unreleased]: https://github.com/atlassian/aws-infrastructure/compare/release-2.20.2...master

## [2.20.2] - 2020-02-04
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the header, we'll add it after release

connection.execute("sudo rm -f $dbconfigXml")

var dbconfigContent = """
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

We can add the XML to src/main/resources and read it from classpath.
This way we'll be able to see XML with proper syntax highlight and verification.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I'll do this

@@ -104,6 +104,7 @@ Resources:
SecurityGroupIds:
- Ref: SshSecurityGroup
- Ref: MySqlSecurityGroup
- Ref: PostgresSecurityGroup
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit shoehorned, but shouldn't have negative consequences AFAIK

@@ -183,6 +184,17 @@ Resources:
FromPort: 3306
ToPort: 3306
CidrIp: 0.0.0.0/0
PostgresSecurityGroup:
Type: AWS::EC2::SecurityGroup
Copy link
Member

Choose a reason for hiding this comment

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

The mini-disadvantage is that it will allocate two Security Groups even tho only one is used. It's an AWS resource, so it'll cost a little and might increase CloudFormation provisioning time. OTOH SecurityGroups are relatively cheap, so I guess we're fine here. Just FYI.

@@ -114,6 +115,39 @@ internal class StandaloneNodeFormula(
connection: SshConnection,
dbconfigXml: String
) {
if (!isMySql) {
connection.execute("sudo rm -f $dbconfigXml")
Copy link
Member

Choose a reason for hiding this comment

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

It nicely explains the current tribal knowledge: the MySQL datasets have predefined dbconfig.xml 👍

@@ -180,6 +181,7 @@ class StandaloneFormula private constructor(
CloseableThreadContext.push("Jira node").use {
key.get().file.facilitateSsh(jiraIp)
}
val isMySql = database is MySqlDatabase
Copy link
Member

Choose a reason for hiding this comment

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

This will cause Database decorators to malfunction. They do exist in the wild.
In that case, isMySql will be false and subsequently we'll delete dbconfig.xml and break that use case.

So let's detect is PostgresDatabase instead, because such callers don't exist yet. This is a short-term hack, so we can still 🤞 for the infra/aws-infra fix.

  • detect is PostgresDatabase instead

@dagguh dagguh requested a review from a team as a code owner February 10, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants