Update readme local development instructions #104
Conversation
It is useful to have a sample of a `.env` that could be sourced to setup a taxjar for development.
The instructions around how environment variables are used in local development needed clarification and simplification. A short blurb on how `solidus-dev-support` handles testing extensions and running local sandbox apps was also added, as these hidden tooling features are not obvious, and have some quirks.
Noah-Silvera
force-pushed
the
update-readme-local-development-instructions
branch
from
3be7740 to
19d9199
Compare
|
@senemsoy would love for you to have a look at this and tell me if it's more clear! |
| * If this is the first time you've run the sandbox, you will notice that a `sandbox` folder is created with a solidus store. | ||
| * **NOTE: If you change the `RAILS_VERSION` or `SOLIDUS_BRANCH` environment variables, you will need to delete this folder before running the tests again** | ||
|
|
||
| ### Releasing a new version |
There was a problem hiding this comment.
@jarednorman I moved this into its own section, but it brought up the question... are these instructions accurate? So far to my memory...
- We update the changelog to display the new version instead of
master - You run the script to release it.
- Is this a secret script? Should we remove these instructions altogether and just keep that knowledge/keys to release limited to you and maybe another person or two?
There was a problem hiding this comment.
The instructions in the README are not accurate, but your instructions in your comment basically are.
gem bump -v [major|minor|patch] -p -t -rThere was a problem hiding this comment.
Using this thing: https://github.com/svenfuchs/gem-release
| ## Development | ||
| ## Development Setup |
There was a problem hiding this comment.
Chris recently linted the README to have hard-wrapped lines at 80 characters. Though it is technically inconsequential, my preference would be to continue doing that.
|
|
||
| ### Running a local Solidus app with TaxJar | ||
|
|
||
| This extension uses the `solidus_dev_support` gem to provide tooling for developing extensions. One of it's most important features is the sandbox app, which runs a solidus store with this extension hooked in. |
There was a problem hiding this comment.
| This extension uses the `solidus_dev_support` gem to provide tooling for developing extensions. One of it's most important features is the sandbox app, which runs a solidus store with this extension hooked in. | |
| This extension uses the `solidus_dev_support` gem to provide tooling for developing extensions. One of its most important features is the sandbox app, which runs a solidus store with this extension hooked in. |
Sorry. 😂
| Before installing dependencies, make sure your environment variables are setup. | ||
| Our CI uses something like this: | ||
| `SOLIDUS_BRANCH=v2.10 DB=postgresql RAILS_VERSION="~> 5.2.0"`. | ||
| The environment variables needed for development are listed in `.env.sample`. Copy these to `.env`, replace the appropriate values, and run `source .env` in your terminal. |
There was a problem hiding this comment.
Curious how others feel about this as a step we encourage for development on the extension. It is quite verbose to specify the full environment before each command, but at least it is explicit. I am worried this may introduce issues for people who are not used to having to source this file every time before running a command in a new shell since we don't use dotenv in this gem.
There was a problem hiding this comment.
Yeah, I might be more inclined to just provide a list of all of the environment variable combinations that are under CI, and then let users copy-paste those as a convenience.
I will personally just be including the environment variables in my commands.
We could always provide dotenv as a development gem, too.
There was a problem hiding this comment.
People should just use direnv. 😅
There was a problem hiding this comment.
I liked one suggestion @jarednorman had which is to update the defaults / fallback for these environment variables to things we consider "sane" in the Gemfile. The defaults we have here don't seem appropriate for the current state of the gem
https://github.com/SuperGoodSoft/solidus_taxjar/blob/master/Gemfile#L5
https://github.com/SuperGoodSoft/solidus_taxjar/blob/master/Gemfile#L12
There was a problem hiding this comment.
These instructions won't work in all shells. 🐟
I think rather than telling people "run this command", it's fine to say set these variables (since there's only one required and one optional.) You can provide the commands to export them in the README, but using a .env file is confusing:
- It might lead anyone familiar with dotenv to think that file would actually be loaded by something, which isn't the case.
- The file doesn't match the format of
.envfiles, but ofdirenvfiles, which would be named.envrc.
Why don't we just tell people to set the two env variables and offer an example command to do so, in case the person doesn't know how
@forkata You might consider using "sensible" instead of "sane". I'd hardly consider the existing defaults "insane". 😅
There was a problem hiding this comment.
@jarednorman sorry for the bad paraphrase. As usual your choice of words is better than mine. I share the same concerns around trying to introduce a dotfile which may imply certain convention or use of a specific gem to manage the environment. If we wanted to provide a shell script for convenience that can be probably made to work independent of the shell someone is using, but even that is a bit too opinionated for me.
I think we should guide developers to what environment they need to export or specify before each command and allow them to set that in any way they wish. I also think this would not be as big of an issue if the defaults we provided actually produced a working gem, which is not the case now.
| export SOLIDUS_BRANCH=v2.10 | ||
| export DB=postgresql | ||
| export RAILS_VERSION="~> 5.2.0" |
There was a problem hiding this comment.
There are already defaults in the Gemfile. If you want to update them you should do that there.
| Before installing dependencies, make sure your environment variables are setup. | ||
| Our CI uses something like this: | ||
| `SOLIDUS_BRANCH=v2.10 DB=postgresql RAILS_VERSION="~> 5.2.0"`. | ||
| The environment variables needed for development are listed in `.env.sample`. Copy these to `.env`, replace the appropriate values, and run `source .env` in your terminal. |
There was a problem hiding this comment.
These instructions won't work in all shells. 🐟
I think rather than telling people "run this command", it's fine to say set these variables (since there's only one required and one optional.) You can provide the commands to export them in the README, but using a .env file is confusing:
- It might lead anyone familiar with dotenv to think that file would actually be loaded by something, which isn't the case.
- The file doesn't match the format of
.envfiles, but ofdirenvfiles, which would be named.envrc.
Why don't we just tell people to set the two env variables and offer an example command to do so, in case the person doesn't know how
@forkata You might consider using "sensible" instead of "sane". I'd hardly consider the existing defaults "insane". 😅
|
We've moved some of the immediately mergeable changes into a new PR: #120 |
|
Some other of the changes have been moved into #121 |
What is the goal of this PR?
The local development instructions were unclear and missing required environment variables, and how to use / the existence of the
solidus-dev-supporttooling is not obvious. This PR intends to correct and ease the setup of your local environment variables, and add a blurb on testing and the sandbox app and the gotchas related tosolidus-dev-support.Merge Checklist