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

Parameters #94

Merged
merged 86 commits into from
Apr 6, 2016
Merged

Parameters #94

merged 86 commits into from
Apr 6, 2016

Conversation

jburel
Copy link
Member

@jburel jburel commented Mar 14, 2016

Add JAVAVER parameters. Default is OpenJDK 1.8 across the various installations. This was not the case.
The first 6 commits were tested by travis in PR #83
Re-organise step installation dependencies.
Update the autogenerate script.

Also Add PGVER (for postgres) and ICEVER (for ice)
cc @hflynn

@joshmoore
Copy link
Member

Two quick points:

  • I was slightly surprised by the mention of SCL? Is this a leftover from our pre-IUS work?
  • As I'm starting to base other repos on this repo, renamings etc. will become breaking changes. Probably manageable for the moment, just something to be aware of.

@jburel
Copy link
Member Author

jburel commented Mar 14, 2016

@joshmoore:

  • At the time of ius work, we decided to keep the scl script but we were not going to expose it in the doc. Happy to remove it we think it is not useful.
  • We might have one more "ice" breaking split, but certainly need to be synchronized before going ahead.

@jburel
Copy link
Member Author

jburel commented Mar 15, 2016

@aleksandra-tarkowska: added option for openjdk17/18-devel as suggested

@jburel jburel force-pushed the java-parameters branch 5 times, most recently from a25b80c to f6e1c1f Compare March 15, 2016 22:08
@snoopycrimecop snoopycrimecop mentioned this pull request Mar 28, 2016
@jburel
Copy link
Member Author

jburel commented Mar 28, 2016

All:
I have started in another branch (on top of that one) to work on the ice3.6 support.If we do not need further changes it might be good to merge.

@atarkowska
Copy link
Member

@pwalczysko are you happy with all the changes? Could this PR being merged as is blocking me

@pwalczysko
Copy link
Member

I was happy before #94 (comment). Are the 3 new commits tested by Travis ? If yes, happy to merge.

@jburel
Copy link
Member Author

jburel commented Mar 30, 2016

@pwalczysko: 2/3 are tested by travis and 149c8a4 tested by @aleksandra-tarkowska

@pwalczysko
Copy link
Member

Happy then, thank you.

@atarkowska
Copy link
Member

This was successfully used in ome/devspace#20.

#start-copy
cp setup_omero_nginx.sh ~omero
#end-copy

#start-install
yum -y --enablerepo=cr install nginx
Copy link
Member

Choose a reason for hiding this comment

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

why do we need CR? EPEL is installed in https://github.com/ome/omero-install/pull/94/files#diff-2722b62c943402fa409b2ef40375cf5dR3

from EPEL you also install nginx/1.6.3

Copy link
Member Author

Choose a reason for hiding this comment

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

I will have to look into it, since we have been splitting several files

Copy link
Member Author

Choose a reason for hiding this comment

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

cr is not required. so it should be removed. The version installed will be 1.6.3.
To match the recommended version cf. ome/omero-documentation#1439, we will have to follow the same approach that the one used for centos 6.
For the rest of the dependencies, we have installed the recommended version
I will update the code according

Copy link
Member

Choose a reason for hiding this comment

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

@jburel I am not sure if I follow but for C6, changing to EPEL will install nginx 1.0.15. I am guessing is better to leave as it is (installing from Nginx repository)

Copy link
Member Author

Choose a reason for hiding this comment

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

for C6, yes the version is old.

We recommend version in our requirements page for java, ice, pg etc. in the doc.
The version installed in the installation script should match what is recommended, for nginx we recommend 1.8/1.9 so that what we should/install test.
That's another reason why we are also thinking of moving the sysadmin doc under omero-install so we will hopefully keep things in synch (it has not been the case e.g pg on ubuntu!)

note that I am currently fixing the ubuntu install script since I just noticed that 1.4.6 is currently installed.

@jburel
Copy link
Member Author

jburel commented Mar 31, 2016

@aleksandra-tarkowska: nginx version installed should now be 1.8+ for centos6/7 and ubuntu1404/debian8 cf. ome/omero-documentation#1439

@jburel
Copy link
Member Author

jburel commented Apr 4, 2016

nginx changes have been tested via travis.
Bumping debian to 1.8 will happen in another PR since it requires some web adjustments (some folders no longer exists e.g. site-available in version 1.8)

@atarkowska
Copy link
Member

@jburel there is one more file that need to be fixed https://github.com/ome/omero-install/blob/develop/linux/omero-systemd.service

This means on Centos7 we have tested

  • both possible values of PGVER

@pwalczysko how did you manage to start server with PostgreSQL 9.5 if pg version is hardcoded in daemon script ?

@jburel
Copy link
Member Author

jburel commented Apr 5, 2016

@aleksandra-tarkowska: Thanks I thought I fixed it. I will try to do that today

@pwalczysko
Copy link
Member

@pwalczysko how did you manage to start server with PostgreSQL 9.5 if pg version is hardcoded in daemon script ?

The comment of mine just claims I have tried both PGVER values. Did not check the Postgres version truly installed.

@joshmoore
Copy link
Member

I will try to do that today

@jburel: Thanks. Let us know though, that could also be a follow on PR.

@sbesson
Copy link
Member

sbesson commented Apr 6, 2016

Since this is also currently consumed (and someway tested) via https://github.com/openmicroscopy/devspace/pull/20/files#diff-91f78839f1da3c73c6a67587b59bc8a5R18, I would be in favor of getting this merged and point to the organization repository for the container installation.

@jburel
Copy link
Member Author

jburel commented Apr 6, 2016

I did not have time yesterday probably better to merge this PR and fix it in a follow-up PR so we can move forward on other fronts

@sbesson
Copy link
Member

sbesson commented Apr 6, 2016

Merging to unlock devspace.

@sbesson sbesson merged commit e0a88e4 into ome:develop Apr 6, 2016
@jburel jburel deleted the java-parameters branch May 26, 2016 13:26
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.

6 participants