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

Cleanup #231

Merged
merged 15 commits into from Mar 7, 2023
Merged

Cleanup #231

merged 15 commits into from Mar 7, 2023

Conversation

beatlevic
Copy link
Collaborator

  • Making ingresses the default of loadBalancers for api and web
  • Separate github workflows for web and api
  • Removed unused volume mounts
  • Removed githooks
  • removed empty .env
  • removed top .dockerignore and expanded the ignores in web and api

Copy link
Collaborator

@andrewplummer andrewplummer left a comment

Choose a reason for hiding this comment

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

This looks good to me. Since you're thinking about this now, you would be an absolute legend if you could also make it possible (and if it's not a huge lift) to choose the instance type when creating a new project (n2, e2, micro etc)...

@@ -9,8 +9,9 @@
"nodePoolCount": 2,
"minNodeCount": 2,
"maxNodeCount": 4,
"machineType": "n2-standard-2"
"machineType": "n2d-standard-2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is n2d supposed to provide better performance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is roughly the same, but cheaper

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This looks good to me. Since you're thinking about this now, you would be an absolute legend if you could also make it possible (and if it's not a huge lift) to choose the instance type when creating a new project (n2, e2, micro etc)...

I'll look into this one, but the list of possible instance types is huge. Before you bootstrap, you can always update the config.json manually :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

but cheaper

Love that part!

but the list of possible instance types is huge
I don't need an exhaustive list... in fact part of the reason it would be useful is for people who don't know what to choose (which also means they wouldn't know what to put in config.json)... ie something this would be ideal (as a CLI select option):

  • e2-micro - Test project or demo, optimize for cost.
  • n2d-standard-2 - Standard baseline with room for growth (default).
  • ??????? - Massive project. Optimize for performance.
  • other - (Type in own...potentially will fail if incorrect or unknown but that's OK).

name: api
port:
number: 80
# rules:
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing to keep in mind here is that the CLI currently I believe references these rules to create features branches (ie feature-api.foo.com etc)... I would like this to still work if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also if not needed then this can just be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe the CLI requires an Ingress if you want to create feature branch and update the rules. I haven't tested this. The current default ( before this PR) is no ingress at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also looking into Gateways which seem to be the successor for Ingresses.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh that's interesting... Well I have some things I want to do with the CLI as well I guess I can make sure this is still working once this is merged and maybe sync with you on gateways 👍

@beatlevic beatlevic merged commit 2040b06 into master Mar 7, 2023
@kaareal
Copy link
Collaborator

kaareal commented Mar 8, 2023

@andrewplummer dont we need the root .env?

@andrewplummer
Copy link
Collaborator

@kaareal I recall either adding or removing it but I can't remember which one. I say apply my main rule: when in doubt get rid of it. If there's a reason to have it then we'll hit it again and whoever does is required to check in an empty file WITH A COMMENT about why to keep it, otherwise it's gone

@kaareal
Copy link
Collaborator

kaareal commented Mar 10, 2023

@andrewplummer i added it back now, it is indeed need,
08fdefc

@andrewplummer
Copy link
Collaborator

Commented in the other PR I think if we fix @bedrockio/config it can actually be removed

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.

None yet

3 participants