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

Create app backend with PG support & E2E Test #1726

Merged
merged 7 commits into from
Jul 27, 2020

Conversation

shmidt-i
Copy link
Contributor

@shmidt-i shmidt-i commented Jul 23, 2020

Prelude:

There's been some discussions around approaches for the DB setup for the backend, e.g. #1531, #1527 and #1598.
While developing, we are using in-memory instance of SQLite, which allows us to iterate fast. But, on the same time we lack checks that stuff actually works together with PostgreSQL (which we plan to use for our demo deployment https://github.com/spotify/backstage/milestone/14 ), as a result - #1719 .
This is a step forward, it doesn't solve all the questions regarding production DB setup, but allows us to develop in a more safer way.

This PR introduces:

  1. Backend package is now part of the backstage-cli create-app (catalog, scaffolder, auth, identity and tech-docs plugins included as of now)
  2. Both packages/example-backend and backend created through cli have support for the PostgreSQL db.
  3. CLI e2e test now includes check for the correct creation and startup of the backend with PostgreSQL db (migrations included).

How to proceed:

  1. See https://github.com/spotify/backstage/pull/1726/checks?check_run_id=905018391
  2. This is exactly error from Backend migrations fail on Postgres for ~v0.1.1-alpha.15 #1719, replicated in the github-actions environment.
  3. Probably easier to go commit by commit
  4. Merge fix Catalog: fix migrations for PG #1729 first (✅)
  5. Check that new build succeeds (✅)
  6. Merge this PR
  7. Profit 😎

@shmidt-i shmidt-i force-pushed the shmidt-i/create-app-backend branch 2 times, most recently from f0f119c to cf1250e Compare July 23, 2020 20:25
@@ -13,6 +13,18 @@ jobs:
build:
runs-on: ${{ matrix.os }}

services:
postgres:
image: postgres:10.8
Copy link
Member

Choose a reason for hiding this comment

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

Why this one specifically? There are less precise (and newer?) ones too right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow you are fast 😎
Mostly created this PR to get the action runs. But feel free to raise things to improve ;)
Here - yes, definitely can go for newer and less precise version

.github/workflows/cli.yml Outdated Show resolved Hide resolved
@@ -34,6 +34,7 @@
"dockerode": "^3.2.0",
"express": "^4.17.1",
"knex": "^0.21.1",
"pg": "^8.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

8 here, 10 in the image - is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is npm package, and before is docker image, idk if think they are THAT correlated?

Copy link
Member

Choose a reason for hiding this comment

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

Me neither, just checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked, they are not correlated, all good

packages/backend/src/index.ts Outdated Show resolved Hide resolved
@shmidt-i shmidt-i force-pushed the shmidt-i/create-app-backend branch 5 times, most recently from 124e7ed to 43211cb Compare July 24, 2020 01:00
@shmidt-i shmidt-i force-pushed the shmidt-i/create-app-backend branch from 43211cb to 5f02544 Compare July 24, 2020 01:16
@shmidt-i shmidt-i marked this pull request as ready for review July 24, 2020 02:05
@shmidt-i shmidt-i requested a review from a team as a code owner July 24, 2020 02:05
@shmidt-i shmidt-i force-pushed the shmidt-i/create-app-backend branch from 48c2933 to a6b8602 Compare July 24, 2020 07:43
AUTH_GITHUB_CLIENT_ID=x AUTH_GITHUB_CLIENT_SECRET=x \
AUTH_OAUTH2_CLIENT_ID=x AUTH_OAUTH2_CLIENT_SECRET=x \
AUTH_OAUTH2_AUTH_URL=x AUTH_OAUTH2_TOKEN_URL=x \
ROLLBAR_ACCOUNT_TOKEN=x \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to go

"@backstage/backend-common": "^{{version}}",
"@backstage/catalog-model": "^{{version}}",
"@backstage/config": "^0.1.1-alpha.13",
"@backstage/config-loader": "^0.1.1-alpha.13",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rugvip do you know why this works only for this particular version and not if I use "^{{version}}" here?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, needs #1734

@@ -16,6 +16,7 @@

const os = require('os');
const fs = require('fs-extra');
const fetch = require('node-fetch');
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 saw whatwg-fetch in the repo, should we use that @benjaminr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies @benjaminr , I meant @benjdlambert 🙃

Copy link
Member

Choose a reason for hiding this comment

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

yeah I was moving more towards using that as it's a one package that can polyfill frontend and backend stuff.

also it plays nicely with msw too. but it needs to be version v2 as v3 causes issues as the react-lazylog depends on a different version which causes issues when compiling the backend because of package hoisting,

Copy link
Contributor Author

@shmidt-i shmidt-i Jul 27, 2020

Choose a reason for hiding this comment

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

Looked into it and it seems like it's actually not both backend and frontend at all :)
And not planning to😜
JakeChampion/fetch#184

try {
await waitFor(() => stdout.includes('Listening on ') || stderr !== '');
if (stderr !== '') {
// Skipping the whole block
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Print the stderr

* Checks every 100ms
* .cancel() is available
* @returns {Promise} Promise of resolution
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the comment

port: process.env.POSTGRES_PORT,
host: process.env.POSTGRES_HOST,
user: process.env.POSTGRES_USER,
password: process.env.POSTGRES_PASSWORD,
Copy link
Member

Choose a reason for hiding this comment

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

Can we grab this from config? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we? I mean secrets in the config, idk, is this what we decided to do?

Copy link
Member

Choose a reason for hiding this comment

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

The config loader has support for secrets from multiple sources, so it's more flexible than this. You can load it from files and whatnot too.

packages/cli/e2e-test/cli-e2e-test.js Outdated Show resolved Hide resolved
import { PluginEnvironment } from '../types';

export default async function createPlugin({ logger }: PluginEnvironment) {
return await createRouter({ logger });
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should start exporting default factories from backend plugins to remove the need for all of this extra wiring in the backend. Kinda a 90% use-case coverage thing. It'll likely also make a config-only deployment easier.

Not in this PR though :p

"packages/*/src",
"plugins/*/src",
"plugins/*/dev",
"plugins/*/migrations"
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

:shipit:

@shmidt-i shmidt-i merged commit 6edbb0c into master Jul 27, 2020
@shmidt-i shmidt-i deleted the shmidt-i/create-app-backend branch July 27, 2020 14:22
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.

4 participants