Skip to content

Local Dev Config#6971

Merged
eric-gade merged 1 commit intomainfrom
eric-https-localdev-config
Sep 16, 2022
Merged

Local Dev Config#6971
eric-gade merged 1 commit intomainfrom
eric-https-localdev-config

Conversation

@eric-gade
Copy link
Contributor

What

This PR updates the Procfile and application.yml files in the repository with commented-out options that can be uncommented and modified for running the application in https locally.

Comments also provide context for how to run https locally and how to use the given options.

I have found that having to go back and forth to several docs to find the appropriate keys and values to use is a bit annoying, and I think this might be a little easier for new people coming on also. LMK what you think.

Procfile Outdated
Comment on lines 3 to 6
Copy link
Contributor

Choose a reason for hiding this comment

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

now this comment is out of date, we can remove

Suggested change
# To test locally with HTTPS, swap the commented
# mailcatcher command below with the currently
# active version. See the README for more
# information on local development

@eric-gade eric-gade force-pushed the eric-https-localdev-config branch from ac5485d to f1d596c Compare September 15, 2022 19:58
Comment on lines 4 to 18
Copy link
Contributor

Choose a reason for hiding this comment

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

should we point to a specific part of the README, like "Testing on a mobile device or in a virtual machine", or make those instructions match there?

I guess what I'm really asking is, how can we make this comment shorter and potentially reduce duplication if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to understand this a bit more:

  1. How is this file even being included, if it's explicitly gitignore'd
  2. When I checked out this branch, it blew away my local application.yml which is quite an inconvenience. Is that going to happen to everyone when it's merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, when I make changes to this file locally, they're marked for commit, which I don't think we'd want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add these comments into application.yml.default instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about we roll back all the application.yml changes, and simply update the README so that the template values can be copied and pasted in the correct place? I definitely don't want to be messing up people's workflows and this might be the best compromise

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that'll work 👍

-- What
This commit updates that Procfile to have mailcatcher check for local
https in development environments and to use the open local IP in such
cases.

Additionally, we provide better detail on how to customize your own
`application.yml` config to enable local dev on https. You can copy
and paste the values and replace your own IP.

changelog: Internal, Local Development, config update and documentation
@eric-gade eric-gade force-pushed the eric-https-localdev-config branch from f1d596c to 50d7d19 Compare September 15, 2022 20:55
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@eric-gade eric-gade merged commit db45b1b into main Sep 16, 2022
@eric-gade eric-gade deleted the eric-https-localdev-config branch September 16, 2022 13:20
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