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.

Sign up for GitHub

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

Jump to bottom

[#12048] Add SQL configuration into build.properties and build-dev.properties #12917

Merged
merged 5 commits into from
Mar 24, 2024
Merged

[#12048] Add SQL configuration into build.properties and build-dev.properties #12917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01a95d1
Add production config
FergusMok Mar 20, 2024
a451ec7
Remove forgotten host and password
FergusMok Mar 20, 2024
bf59b3e
Fix lint
FergusMok Mar 20, 2024
309a710
Merge branch 'master' into add-production-postgres-config
FergusMok Mar 20, 2024
c078282
Merge branch 'master' into add-production-postgres-config
ziqing26 Mar 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions src/main/java/teammates/common/util/Config.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,21 @@ public final class Config {
/** The value of the "app.frontend.url" in build.properties file. */
public static final String APP_FRONTEND_URL;

/** The value of the "app.postgres.host" in build.properties file. */
public static final String POSTGRES_HOST;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can standardise the property to either not be prefixed by "app-" (like here) or prefix the property with "app-" like in your src/main/resources/build-dev.template.properties

On a side note would you know what "app-" is? I'm wondering if it means its a property of GAE, else we probably drop the "app" from postgres attributes since its on Cloud SQL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the lack of standardization is quite strange too (I flip flopped between both a few times)

For the variable name, I've prefixed without "APP" to be more in-line with other variables for other services (E.g EMAIL_SENDEREMAIL, MAILGUN_APIKEY). The only variables with "APP" prefix seem to for truly "application" details, like APP_FRONTEND_URL, APP_ADMINS.

For the property prefix, every single property has the 'app' prefix. I believe it's a matter of standardization only, and it doesn't contribute to functionality.

Since it affects many other variables, should we handle this in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@NicolasCwy Maybe can open an new issue (which can be labelled as good first issue) for this? Let's merge this in first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup was just thinking of changing this one property, but I'll create an issue anyway


/** The value of the "app.postgres.port" in build.properties file. */
public static final String POSTGRES_PORT;

/** The value of the "app.postgres.databasename" in build.properties file. */
public static final String POSTGRES_DATABASENAME;

/** The value of the "app.postgres.username" in build.properties file. */
public static final String POSTGRES_USERNAME;

/** The value of the "app.postgres.password" in build.properties file. */
public static final String POSTGRES_PASSWORD;

/** The value of the "app.production.gcs.bucketname" in build.properties file. */
public static final String PRODUCTION_GCS_BUCKETNAME;

Expand Down Expand Up @@ -100,18 +115,6 @@ public final class Config {
/** The value of the "app.localdatastore.port" in build-dev.properties file. */
public static final int APP_LOCALDATASTORE_PORT;

/** The value of the "app.localpostgres.port" in build-dev.properties file. */
public static final int APP_LOCALPOSTGRES_PORT;

/** The value of the "app.localpostgres.username" in build-dev.properties file. */
public static final String APP_LOCALPOSTGRES_USERNAME;

/** The value of the "app.localpostgres.password" in build-dev.properties file. */
public static final String APP_LOCALPOSTGRES_PASSWORD;

/** The value of the "app.localpostgres.db" in build-dev.properties file. */
public static final String APP_LOCALPOSTGRES_DB;

/** The value of the "app.enable.devserver.login" in build-dev.properties file. */
public static final boolean ENABLE_DEVSERVER_LOGIN;

Expand Down Expand Up @@ -158,6 +161,11 @@ public final class Config {
CSRF_KEY = getProperty(properties, devProperties, "app.csrf.key");
BACKDOOR_KEY = getProperty(properties, devProperties, "app.backdoor.key");
PRODUCTION_GCS_BUCKETNAME = getProperty(properties, devProperties, "app.production.gcs.bucketname");
POSTGRES_HOST = getProperty(properties, devProperties, "app.postgres.host");
POSTGRES_PORT = getProperty(properties, devProperties, "app.postgres.port");
POSTGRES_DATABASENAME = getProperty(properties, devProperties, "app.postgres.databasename");
POSTGRES_USERNAME = getProperty(properties, devProperties, "app.postgres.username");
POSTGRES_PASSWORD = getProperty(properties, devProperties, "app.postgres.password");
BACKUP_GCS_BUCKETNAME = getProperty(properties, devProperties, "app.backup.gcs.bucketname");
ENCRYPTION_KEY = getProperty(properties, devProperties, "app.encryption.key");
AUTH_TYPE = getProperty(properties, devProperties, "app.auth.type");
Expand Down Expand Up @@ -186,10 +194,6 @@ public final class Config {
// The following properties are not used in production server.
// So they will only be read from build-dev.properties file.
APP_LOCALDATASTORE_PORT = Integer.parseInt(devProperties.getProperty("app.localdatastore.port", "8484"));
APP_LOCALPOSTGRES_PORT = Integer.parseInt(devProperties.getProperty("app.localpostgres.port", "5432"));
APP_LOCALPOSTGRES_USERNAME = devProperties.getProperty("app.localpostgres.username", "teammates");
APP_LOCALPOSTGRES_PASSWORD = devProperties.getProperty("app.localpostgres.password", "teammates");
APP_LOCALPOSTGRES_DB = devProperties.getProperty("app.localpostgres.db", "teammates");
ENABLE_DEVSERVER_LOGIN = Boolean.parseBoolean(devProperties.getProperty("app.enable.devserver.login", "false"));
TASKQUEUE_ACTIVE = Boolean.parseBoolean(devProperties.getProperty("app.taskqueue.active", "true"));
}
Expand Down Expand Up @@ -302,7 +306,7 @@ public static boolean isUsingFirebase() {
* Returns db connection URL.
*/
public static String getDbConnectionUrl() {
return "jdbc:postgresql://localhost:" + APP_LOCALPOSTGRES_PORT + "/" + APP_LOCALPOSTGRES_DB;
return String.format("jdbc:postgresql://%s:%s/%s", POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASENAME);
}

public static boolean isUsingSendgrid() {
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/teammates/ui/servlets/HibernateContextListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ public class HibernateContextListener implements ServletContextListener {
@Override
public void contextInitialized(ServletContextEvent event) {
// Invoked by Jetty at application startup.
HibernateUtil.buildSessionFactory(Config.getDbConnectionUrl(), Config.APP_LOCALPOSTGRES_USERNAME,
Config.APP_LOCALPOSTGRES_PASSWORD);
HibernateUtil.buildSessionFactory(Config.getDbConnectionUrl(), Config.POSTGRES_USERNAME, Config.POSTGRES_PASSWORD);
}

@Override
Expand Down
7 changes: 7 additions & 0 deletions src/main/resources/build-dev.template.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ app.frontend.url = http\://localhost:4200
# This is the port to connect with local Datastore emulator.
app.localdatastore.port = 8484

# Configurations for postgres
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe can add more explanation here on what values these fields should be set to (e.g. These correspond to the postgres instance details in the docker image and no change required etc)

app.postgres.host=localhost
app.postgres.port=5432
app.postgres.databasename=teammates
app.postgres.username=teammates
app.postgres.password=teammates

# This indicates whether task queues are active (e.g. items added to task queue will be queued for execution).
# This flag is only used during development mode; in production, task queue will always be active.
# In addition, during development mode, there is no "queueing", i.e. all tasks will be immediately executed.
Expand Down
9 changes: 9 additions & 0 deletions src/main/resources/build.template.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,15 @@ app.version = 8-0-0
# when running locally, the default domain name is http://localhost:8080
# app.frontend.url=

# This is the Google Cloud SQL Configurations for production purposes.
# Using the Severless VPC Access, host should be Private IP of the SQL instance.
# For dev server, leaving it blank will do.
app.postgres.host=
app.postgres.port=5432
app.postgres.databasename=postgres
app.postgres.username=postgres
app.postgres.password=

# This is the Google Cloud Storage bucket name used by the app for production purposes, e.g. user profile pictures.
# For dev server, any name will do.
# For staging server, if you use the default bucket for your project, it should be <your app id>.appspot.com
Expand Down
Loading