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

Config should live in entry point file, use a specified shape and type checking. #3

Open
a-bigelow opened this issue Jan 8, 2022 · 14 comments

Comments

@a-bigelow
Copy link
Contributor

a-bigelow commented Jan 8, 2022

To start, I am offering to implement this refactor -- gauging interest here first.

Currently the config for the server lives in a config.ts file that the user must copy from the sample.

What I propose is for stack config to live in the CDK App entry point file, in this case server-hosting.ts. This would look similar to the below:

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import { ServerHostingStack } from './server-hosting-stack';

const app = new cdk.App();
new ServerHostingStack(app, 'ServerHostingStack', {
  env: { account: Config.account, region: Config.region},
  prefix: 'SatisfactoryHosting',
  restartApi: true,
  useExperimentalBuild: false,
  bucketName: 'someBucket',
  vpcId: 'vpc-abc123youandme',
  subnetId: 'subnet-blablabla',
  availabilityZone: 'us-east-1a',
});

To get this to work, we just need to implement an extension of the StackProps interface:

interface satisfactoryStackProps extends StackProps {
  /**
   * Prefix for all named components of this stack.
   */
  prefix: string,
  /**
   * Whether or not to create the server-restart API
   */
  restartApi: boolean,
  /**
   * Whether or not to use the Satisfactory Experimental Build
   */
  useExperimentalBuild: boolean,
  /**
   * Name of the S3 buckt to use for server backups.
   * @default - A new bucket is created and used.
   */
  bucketName?: string,
  /**
   * ID of the VPC in which to host the Instance.
   * @default - the default VPC for your given region is used.
   */
  vpcId?: string,
  /**
   * The subnet where your instance will be placed.
   * NOTE: If you have already defined availabilityZone, this is redundant.
   * @default - a random public subnet from your VPC is selected.
   */
  subnetId?: string,
  /**
   * The Availability zone in which to host your server.
   * NOTE: If you have already defined subnetId, this is redundant.
   */
  availabilityZone?: string,
}

Then we tell the stack itself to use that interface instead, and we're good to go.

export class ServerHostingStack extends Stack {
  constructor(scope: Construct, id: string, props?: satisfactoryStackProps) {
    super(scope, id, props);

Let me know your thoughts. Happy to implement this (or a version of it).

@feydan
Copy link
Owner

feydan commented Jan 8, 2022

@a-bigelow I like the idea of creating an interface for the Config.ts shape so that when the file is copied over and modified, it is clearer if you specify an incorrect type.

However, I do have a couple concerns with your proposal.

  1. Having the config file not checked in to source control allows you to pull latest changes without having to touch your config file, and assuming your config.ts shape is correct, it should work. This also helps prevent accidentally checking in potentially sensitive data.
  2. It looks like props is already using the type StackProps, which is different than the satisfactoryStackProps shape. I'm not sure this would work properly with CDK.

@a-bigelow
Copy link
Contributor Author

@feydan Points taken, couple notes:

  1. I think part of this pain is from not distributing this as a construct rather than a stack -- ideally if the goal is to distribute a reusable component and let people pull updates to it without overwriting their config, releasing it as a CDK construct via NPM is going to cause less friction.
  2. In my proposal, satisfactoryStackProps is an extension of StackProps, and will meet all the requirements as a result. This is the pattern I use in all of my other constructs related work.

@feydan
Copy link
Owner

feydan commented Jan 8, 2022

Ok I think I see where you are going with this now. So if we assume this is in a npm package, what do I now have to do as a user to get this working? something like this?

  1. Create a new empty project
  2. Npm install this project
  3. Create server-hosting.ts with your example code (const app = new cdk.App(); new ServerHostingStack....)
  4. npx cdk deploy
    ?

@a-bigelow
Copy link
Contributor Author

More or less, it would like a bit like this:

#!/usr/bin/env node
import 'source-map-support/register';
import * as cdk from 'aws-cdk-lib';
import { SatisfactoryDedicatedServer } from 'satisfactory-aws-cdk';

class ServerHostingStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const myServer = new SatisfactoryDedicatedServer(this, 'myServer', {
         prefix: 'SatisfactoryHosting',
         restartApi: true,
         useExperimentalBuild: false,
         bucketName: 'someBucket',
         vpcId: 'vpc-abc123youandme',
         subnetId: 'subnet-blablabla',
         availabilityZone: 'us-east-1a',
   });
};

const app = new cdk.App();
new ServerHostingStack(app, 'ServerHostingStack', {
  env: { account: process.env.CDK_DEFAULT_ACCOUNT, region: process.env.CDK_DEFAULT_REGION },
};

Then when updates are released for the construct, all you really need to do (or optionally not do) is update the NPM package and update your stack. Your config won't change, and you'll get whatever new features/fixes have been released.

@a-bigelow
Copy link
Contributor Author

This does get into having to show people how to setup CDK projects a bit, and assumes some basic TS knowledge, so we could also create a Projen extension so people could just run npx projen new --from aws-satisfactory-cdk to do all the basic setup.

@a-bigelow
Copy link
Contributor Author

a-bigelow commented Jan 9, 2022

@feydan I got bored so here's a prototype

It's not released anywhere and if you decide you'd rather not have your source code appropriated elsewhere, I'll respectfully remove the repo.

@feydan
Copy link
Owner

feydan commented Jan 9, 2022

This is pretty slick, and feel free to go crazy with the code outside of this project . I'd be totally happy if you/someone else wants to build off of and/or help maintain it.

I think this would be a good addition, as it removes git as a dependency and makes it more extensible (i.e. if someone wanted to manage multiple satisfactory servers). If you want to put it in an MR, I'm happy to take a look and test it out. We would need to change up the readme instructions too.

@twitu
Copy link
Collaborator

twitu commented Jan 9, 2022

I think there are a couple of parts here.

Adding an interface for the config object is definitely a good to have. It'll make deployments hard to get wrong and will make it easier to extend for use cases like "multiple satisfactory servers".

I still feel that having configuration in a separate from file from the logic/app parts of it is a good idea, because it makes the experience of using this most like that of an executable. Just consider the executable an opaque box, change the config and run.

import * as cdk from 'aws-cdk-lib';
import { Config } from './config.ts';
import { ServerHostingStack } from './server-hosting-stack';

const app = new cdk.App();
new ServerHostingStack(app, 'ServerHostingStack', {
  env: { account: Config.account, region: Config.region},
  ...Config
});

// If somebody wants to extend this for multiple-servers or other use cases it could be like.

import { TeamConfig } from './config.ts';

new TeamServerHostingStack(app, 'TeamServerHostingStack', {
  env: { account: Config.account, region: Config.region},
  ...TeamConfig
});

About the part of publishing an npm package and projen extension. I feel that it would not add significant value mainly because packaging and distributing as a library is super useful if the it is going to be included as part of a larger solution. This server setup currently more like a standalone thing (feels more like a cli command or an executable) that is used to deploy and destroy the infra.

Still that's just my reservation, if there is a target audience that needs the distribution please go ahead.

@a-bigelow
Copy link
Contributor Author

@twitu Overall in my experience it's just easier to share out a construct rather than a stack. It makes versioning easier and removes the requirement of understanding git at all, like @feydan said. Granted, understanding npm isn't exactly a smaller requirement.

The nice thing about TS is that ultimately it doesn't matter where the config lives, and especially if the package is released via NPM then ultimately it's up to the user. They can either import their config and reference it as you've shown above, or just put the interface options in the constructor.

@twitu
Copy link
Collaborator

twitu commented Jan 9, 2022

Hmm yeah when you put it this way distributing as an npm package does have advantages. So if were to publish one. Then this repo would actually have two projects -

  • Server hosting library which would be published as a package that audience can consume in their own cdk apps
  • Server hosting cdk application that users would actually use to deploy their setup. The application would consume the construct defined in the above library.

I'm not sure how such projects are structured in the node ecosystem. Should the application consume the local definition of server hosting construct? Or should it import from the npm package library?

removes the requirement of understanding git

Just to clarify on this. The current setup does not really require understanding or using git because the files can be downloaded as a zip and then there are no git related commands in the deployment.

@a-bigelow
Copy link
Contributor Author

Ah yeah, I routinely forget that people can download repos as zip since I never do it. Point taken.

I would advocate for this structure overall, and both of these would need their own repositories:

  • A CDK Construct for the satisfactory server stack
  • An extension of the Projen awscdk-app-ts project type that synthesizes the required elements for the satisfactory server stack, including a base config file.

If Projen didn't exist, I would agree that distributing the stack like you've done in this repository is the easiest way to do this, but once a Projen type exists, all the user needs to do is projen new --from <our-project-type-npm-package> and Projen takes care of the rest. Really the only requirement at that point is installing nodejs/npm. And to clarify, the stack generated by Projen would consume the NPM package of the CDK construct since they would live in separate repos.

@feydan
Copy link
Owner

feydan commented Jan 9, 2022

Is there any way to support the config file method as well as the cdk construct in this single repo?

@a-bigelow
Copy link
Contributor Author

I'm sure there's a way to host both a stack and a construct in the same repo, not sure it's worth the headache dealing with the CI though.

The projen awscdk-construct project type comes with a bunch of pre-baked CI workflows, plus dealing with part of a repo using projen and part of it not using projen just sounds like a bad time to me. I've already started the process of releasing a construct via this repo, and was pretty close to generating a release before some recent colors.js drama broke the CI pipeline.

@a-bigelow
Copy link
Contributor Author

Update: I was able to fire off a release once all the colors.js crap figured itself out. Definitely @experimental but you can check out the repo here

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

No branches or pull requests

3 participants