-
Notifications
You must be signed in to change notification settings - Fork 32
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
Made taxbrain install script more usable #817
base: master
Are you sure you want to change the base?
Conversation
Thanks for re-working the I'm going to to need some time to go over this and make sure everything is still working as expected. |
deploy/install_taxbrain_server.sh
Outdated
@@ -67,7 +67,7 @@ if prompt_user "Install conda requirements?"; then | |||
fi | |||
done | |||
# Places these packages last, to install taxcalc, etc. at the end. | |||
PACKAGES+=('btax' 'ogusa') | |||
PACKAGES+=('btax' 'ogusa' 'taxcalc') | |||
echo "conda install $CHANNEL ${PACKAGES[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you still specify which version of these packages that we want to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do that after the script runs, or by changing the script. I guess right now it just gets the most recent one available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be able to specify the version in the script. It's pretty common to have to pin to an older version. Each parameter change and API change requires a change on the PolicyBrain side. These tend to happen a lot. And on occasion they break something on the PolicyBrain side that we don't have time to solve at the moment.
.travis.yml
Outdated
@@ -8,6 +8,7 @@ install: | |||
- wget http://repo.continuum.io/miniconda/Miniconda-latest-Linux-x86_64.sh -O miniconda.sh | |||
- bash miniconda.sh -b -p $HOME/miniconda | |||
- export PATH="$HOME/miniconda/bin:$PATH" | |||
- python -c 'import os,sys,fcntl; flags = fcntl.fcntl(sys.stdout, fcntl.F_GETFL); fcntl.fcntl(sys.stdout, fcntl.F_SETFL, flags&~os.O_NONBLOCK);' | |||
- pushd deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a fun one. See #799
This PR looks good. Once you add back the capability to set the taxcalc, btax, and ogusa versions, this should be good to go. @GoFroggyRun can you take a look at this PR? |
@codekansas thanks for simplifying the installation script as well as providing those documentation. I don't have much technical output regarding the script, but was wondering whether it‘d make sense to update |
@hdoupe I believe it just uses the versions of those that are in the various requirements files. I'd thought there was something important about the install order (which there seemed to be from the previous script) but it doesn't seem to be the case. @GoFroggyRun It shouldn't change anything, as it just gives the user more options on what they want to do and makes things download faster. |
@Kpinkelman would you mind taking a look at this script? Here are a few things you can check:
Run these tests with |
I had a lot of difficulty hard-resetting my environment (partly being on a slow internet connection) and I figured it would be good to make some changes to the install script. Mainly:
yes | ./install_taxbrain_server.sh
set -e
option - this avoids a bunch of the|| return 1
stuff from the previous script and is probably a more sane way to do thingsI also noticed that there were a lot of redundant
source activate aei_dropq && source webapp_env.sh
so I added the first part to thewebapp_env.sh
script, so you only have to dosource webapp_env.sh
. Maybe there was a reason for keeping them separate? I'm not sure. @hdoupe