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

3937 rewrite installer in python again #6484

Merged
merged 23 commits into from
Jan 13, 2020

Conversation

landreev
Copy link
Contributor

@landreev landreev commented Jan 6, 2020

What this PR does / why we need it:
New Installer Implementation, in Python
Which issue(s) this PR closes:

Closes #3937

Special notes for your reviewer:

Suggestions on how to test this:

Does this PR introduce a user interface change?:

Is there a release notes update needed for this change?:

Additional documentation:

@coveralls
Copy link

coveralls commented Jan 6, 2020

Coverage Status

Coverage decreased (-0.009%) to 19.504% when pulling a35e3f6 on 3937-rewrite-installer-in-python-again into b70cd18 on develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks good so I'm sending it to QA. That said, her are some (minor) suggestions, questions, and comments. @landreev should feel free to pull this out of QA if he wants to work on any of this.

Suggestions:

  • in the Perl install script, mention the new Python script
  • in requirements.txt, put version of psycopg2
  • consider recommending Python 3's venv (virtual environment)

Questions:

  • 10.5072 is configured but when they contact DataCite do they get a new authority for testing?
  • Any thoughts on when we'll stop supporting the Perl installer?

Comments:

@djbrooke
Copy link
Contributor

djbrooke commented Jan 6, 2020

@donsizemore, @landreev mentioned that you were discussing this quite a bit. Your review here would be very appreciated if you have the time to do so. Thanks in advance!

@donsizemore
Copy link
Contributor

@landreev what's the relationship between default.config and interactive.config? I popped the latter into Ansible as a template but the install script seems to want default.config's structure instead.

I can swap out the templates, but default.config has 30 lines of content while interactive.config has 47. It's looking like I'll need both?

@landreev
Copy link
Contributor Author

landreev commented Jan 8, 2020

@landreev what's the relationship between default.config and interactive.config? I popped the latter into Ansible as a template but the install script seems to want default.config's structure instead.

I can swap out the templates, but default.config has 30 lines of content while interactive.config has 47. It's looking like I'll need both?

(@pdurbin mentioned he had already answered this, but just to clarify and document this here:)

Yes, the script needs both.
The default.config is for supplying the configuration values. The installer also has a new option --config_file= that can be used to use a custom file, instead of the default.config.

intreractive.config stores the prompts and info/help messages for the interactive setup dialog. It's the stuff that's hard-coded in %CONFIG_PROMPTS and %CONFIG_COMMENTS in the old installer. Storing it in a config file outside the script should make it cleaner/easier to maintain (or even localize the installer in a different language, if anyone ever feels a need for that...).

Thinking about it, there's no need to load and parse this file at all, when the installer is running in the non-interactive mode. I'll add an if statement for that.

@donsizemore
Copy link
Contributor

donsizemore commented Jan 8, 2020

@landreev one more question: previously the dataverse installer captured the output of glassfish-setup into glassfish-setup.out — do we want to keep that, or offer an output file flag? helpful for debugging...

@poikilotherm
Copy link
Contributor

poikilotherm commented Jan 8, 2020

Nobody asked for it, but my 2 cents: as long as @donsizemore is not relying on the OpenShift workarounds, feel free to let anything related to it RIP in good ol' Perl version.

@pdurbin and I where going to cut out the Openshift stuff from the guide anyway and move all cloud/container regarding OpenShift to dataverse-k8s. See also gdcc/dataverse-kubernetes#129

@pdurbin
Copy link
Member

pdurbin commented Jan 8, 2020

Oh, I guess I'll throw in a few cents too. Back when the (Perl) installer required root I wrote a little shell script called "devinstall" at f181543 that seemed to work for me as an alternative. These days you can also find a slight variation of this script at https://github.com/IQSS/dataverse/blob/v4.18.1/scripts/deploy/phoenix.dataverse.org/install

These days I just use the standard installer for dev environments rather than my "devinstall" script. But on servers, I use dataverse-ansible, which doesn't use the old Perl script. I guess if dataverse-ansible uses the new Python script I'll start using it on servers.

I'm not exactly sure where I'm going with this. I guess I'm happy that the new Python script will be exercised regularly by dataverse-ansible. I think somewhere in docker-aio there's an option to use the old Perl script but I'm not in the habit of using docker-aio. In short, I'm glad the new code will be tested regularly, at least the non-interactive mode.

@kcondon kcondon self-assigned this Jan 9, 2020
@donsizemore
Copy link
Contributor

Screen Shot 2020-01-09 at 14 46 08

dataverse-ansible now deploys the 4.18.1 release normally using the python3 installer instead of a bunch of calls I had broken out into Ansible-ese. I did keep the JDBC jar placement in Ansible's realm as that's a configurable switch for testing of newer jars. If anyone would like to give the work-in-progress a whirl, it's https://github.com/IQSS/dataverse-ansible/tree/139_python3_installer

@kcondon
Copy link
Contributor

kcondon commented Jan 9, 2020

Issues found so far:

  1. Add to readme: yum install postgresql96-devel needed so .h files available for pip install psycopg2
  2. default.config from original non-interactive installer has same name as default.config used for python install and may conflict, resulting in failed install if downloaded from doc.
  3. *.config files for python missing from dvinstall.zip
  4. Replace default system email, nobody@mailinator.com, with blank
  5. Maybe tighten up README to indicate supported python versions for psycopg: http://initd.org/psycopg/docs/install.html v2.7 or v3.4-3.8
  6. Need python-devel to be installed for Python.h to build psycopg.

@landreev
Copy link
Contributor Author

landreev commented Jan 9, 2020

Yay! I'll give it a try.

@donsizemore

dataverse-ansible now deploys the 4.18.1 release normally using the python3 installer instead of a bunch of calls I had broken out into Ansible-ese.

@pdurbin
Copy link
Member

pdurbin commented Jan 9, 2020

@donsizemore wow! Looking at https://github.com/IQSS/dataverse-ansible/compare/139_python3_installer I see you've been busy!! Thanks!! 🎉

@kcondon kcondon assigned landreev and unassigned kcondon Jan 10, 2020
@kcondon
Copy link
Contributor

kcondon commented Jan 10, 2020

All set testing, passing back to dev.

@landreev
Copy link
Contributor Author

@kcondon, I addressed the feedback below. With the exception of the old-style default.config - I figured it wasn't necessary, just because of how unlikely it is to happen to anyone else. (and easy to fix if it does :)

Issues found so far:

1. Add to readme: yum install postgresql96-devel needed so .h files available for pip install psycopg2

2. default.config from original non-interactive installer has same name as default.config used for python install and may conflict, resulting in failed install if downloaded from doc.

3. *.config files for python missing from dvinstall.zip

4. Replace default system email, [nobody@mailinator.com](mailto:nobody@mailinator.com), with blank

5. Maybe tighten up README to indicate supported python versions for psycopg: http://initd.org/psycopg/docs/install.html v2.7 or v3.4-3.8

6. Need python-devel to be installed for Python.h to build psycopg.

@landreev
Copy link
Contributor Author

@pdurbin

* Any thoughts on when we'll stop supporting the Perl installer?

My thought was to release it as beta/experimental in the next release; invite people to give it a try; incorporate any feedback we receive and then switch to it as the default installer for the release after that.
But idk - I'm feeling a little discouraged by the fact that a user needs to do more work to run the new installer than the old one. For ex., having to install psycopg2 - at least some people will definitely run into some issues with that.
By which I mean, none of it is going to be a problem for another developer. But not everybody trying to install Dataverse from a release bundle is a developer, or super technical necessarily.

This is something we may want to discuss as a team at some point during the next release cycle. I personally would be ok to say that you have to be somewhat of a sysadmin/be comfortable with python, etc. to install Dataverse.

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.

Rewrite installer in Python 3
7 participants