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

isProduction replaced by isDevLocal #1892

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brunobowden
Copy link
Collaborator

  • Removed app server type enums and urls

How did you test the change?

TBD: Staging server

Checklist:

- Removed app server type enums and urls
Copy link
Contributor

@matthewblain matthewblain left a comment

Choose a reason for hiding this comment

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

Confusingly, the the Environment class uses two entirely different meanings for "dev" and "prod".

Meaning 1: App Engine.
Development = dev_appserver. Running on your local workstation. Simulates most (but not all) of the rest of the platform. Has access to the internet and thus potentially parts of some cloud project, given appropriate authorization and configuration (e.g. pick a firebase project and you might be able to connect).
Production = Google Cloud Platform. Running 'on the cloud', and in the context of a cloud project, with access to all of those resources.

Meaning 2: The various deployments of this code. Here's my anaylysis, I don't know if this was documented when it was created?
Production : Deployed to 'who-mh-prod', a particular instance which has public traffic routed to it, a more stringent set of requirements (e.g. around security and privacy and deployment), access to particular resources.
Staging: Deployed to who-mh-something. Deployed automatically? Has ??? requirments, etc; used primarily for testing by the team, may have some shared testing resources (e.g. firebase?) for other testing deployments.
Hack: Deployed to who-mh-something. Publicly accessible for testing.
Test, development: I'm not quite sure.
???: Individual developer's deployments to GCP. For example, I have a copy of this code deployed to my own GCP project. I have to make local edits to my app to point to it, but otherwise it's a functional staging enviornment. Is this what test or development were meant for?
dev_local: Running on dev_appserver. Note that the appid here is also simulated and can be set to any value.

I expect that these various distinctions might be useful in different ways, but I'm not sure. Certainly could be expressed in this class as explicit use cases? E.g. perhaps only two are needed right now: "IsProdInstance", "IsDevAppserver". One would be used to restrict things as needed (e.g. disable some sort of debug or logging code), the other to enable 'insecure' things for testing or reconfigure communication needed for dev_appserver to contact, say, the 'staging' firebase server.

So for example much of this logging is really nice to have running on GCP (prod definition 1), but might not be desirable on the 'prod' (defniition 2) instance.

@brunobowden
Copy link
Collaborator Author

We should minimize the variation between servers as it adds to complexity. By eliminating isProduction it means that the hack, prod and staging servers should be closer to identical. I think the only one that needs special casing is the dev_local where there's some differences, e.g. cron jobs and access to the Firebase staging server.

So Matthew, I think this is good for now.

@matthewblain
Copy link
Contributor

I'm in favor of simplification. I just don't think this is the correct simplification. Notably, I'd either fully delete all of these logging statements, or make them unconditional.
Dev_appserver is rareley where you care about logging--or if you do, you can easily add it manually.
In prod... well, there's this which I haven't tried yet!
https://cloud.google.com/blog/products/gcp/stackdriver-debugger-add-application-logs-on-the-fly-with-no-restarts

@stale
Copy link

stale bot commented Jan 6, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolved:stale No recent activity on the issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants