-
Notifications
You must be signed in to change notification settings - Fork 54
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
Comments
@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.
|
@feydan Points taken, couple notes:
|
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?
|
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. |
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 |
@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. |
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. |
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. |
@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 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 |
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 -
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?
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. |
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:
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 |
Is there any way to support the config file method as well as the cdk construct in this single repo? |
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 |
Update: I was able to fire off a release once all the colors.js crap figured itself out. Definitely |
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:To get this to work, we just need to implement an extension of the
StackProps
interface:Then we tell the stack itself to use that interface instead, and we're good to go.
Let me know your thoughts. Happy to implement this (or a version of it).
The text was updated successfully, but these errors were encountered: