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

mrc-6062 Skeleton app #1

Merged
merged 18 commits into from
Dec 9, 2024
Merged

mrc-6062 Skeleton app #1

merged 18 commits into from
Dec 9, 2024

Conversation

EmmaLRussell
Copy link
Contributor

@EmmaLRussell EmmaLRussell commented Nov 27, 2024

This branch implements a basic express server that doesn't do anything except return package version. It includes:

  • build and run scripts
  • basic code structure: routes, controller, logging
  • unit and integration tests (currently run from the same script, but these could be separated later).
  • lint
  • docker build
  • basic github workflows

The next branch will include some example datasets and returning tile data.

To do, in subsequent tickets: coverage and error handling

import morgan from "morgan";
import { Dict } from "./types";

export const initialiseLogging = (app: Application) => {
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 format largely stolen from WODIN.

@EmmaLRussell EmmaLRussell marked this pull request as ready for review December 2, 2024 14:08
Copy link
Contributor

@absternator absternator left a comment

Choose a reason for hiding this comment

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

Looks great!!! config is great.. just left a few comments

registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}
- name: Build docker image
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably be testing code first before pushing? because then we create bad images

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 just builds and pushes the image for the SHA tag though, and then we run that for the integration tests. The branch tag only gets pushed once the tests pass, so branch tag should always be good. We've used this pattern elsewhere.

Copy link
Contributor

@david-mears-2 david-mears-2 left a comment

Choose a reason for hiding this comment

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

It works for me. I have questions, and a comment about ConfigReader.

this.rootDir = rootDir;
}

readConfigFile(...filePath: string[]): object | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

if the function / class is for reading config files, should it be allowed to have parameters? I think the filePath could be something known by ConfigReader. are we expecting there to be other config files to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debatable I think. This class was largely ripped off from WODIN where we certainly does have multiple config files. We may end up with multiple config files in grout, I think it's still tbd.

Either way I think it's ok for the server module to centralise knowledge of where all the folders are. That's the approach I've been taking anyway!


export class IndexController {
static getIndex = (_req: Request, res: Response) => {
const version = process.env.npm_package_version;
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this value get into the process.env object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Through the magic of running from an npm script :)
https://docs.npmjs.com/cli/v9/using-npm/scripts#packagejson-vars


export const initialiseLogging = (app: Application) => {
const customFormat = (
tokens: Dict<(req: Request, res: Response, header?: string) => string>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not able to parse this - what does 'tokens' refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It refers to the bits of information relating to a request/response pair which can be logged by morgan (the logging package). As you can see from the type, these are actually functions, and this whole customFormat method is passed to morgan to tell it how to log requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing, but I guess all is explained in the morgan docs.

@EmmaLRussell EmmaLRussell merged commit 7bc3651 into main Dec 9, 2024
2 checks passed
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.

3 participants