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

Fix OMERO version number generation #366

Merged
merged 4 commits into from May 19, 2013
Merged

Fix OMERO version number generation #366

merged 4 commits into from May 19, 2013

Conversation

@sbesson
Copy link
Member

sbesson commented May 16, 2013

Changes

  • Fix a bug in the server upgrade page where the previous version number was
    calculated to 5.-1
  • Correctly parse the version numbers using semver.org conventions
  • Define list of previous minor versions for each major version
  • Do not import the re module globally

Testing instructions:

  • Check the build is green and the server upgrade page is correctly updated

  • Locally, test the OMERO documentation build with multiple release numbers, e.g.

    cd omero
    OMERO_RELEASE=5.0.0 make html
    OMERO_RELEASE=5.0.0-alpha3 make html
    OMERO_RELEASE=5.0.1 make html
    OMERO_RELEASE=5.1.0 make html
    OMERO_RELEASE=5.1.5 make html
    OMERO_RELEASE=5.100.0 make html
    make html
    

    After each build, look at the _build/html/sysadmins/server-upgrade.html page especially the _build/html/sysadmins/server-upgrade.html#upgrade-your-database section and check everything behaves as expected

  • Check the following build command

    cd omero
    OMERO_RELEASE=6.0.0 make html
    

    fails with an informative error message. NB: for each new major series as 5.0.x, the previous minor version number is read from a hard-coded function. If anyone has a better suggestion, I am happy to hear about it (I deliberately didn't introduce an extra environment variable)

sbesson added 2 commits May 16, 2013
- Fix a bug in the server upgrade page where the previous version number was
calculated to 5.-1
- Correctly parse the version numbers using semver.org conventions
- Define list of previous minor versions for each major version
- Do not import the re module globally

# Define Sphinx version and release variables and development branch
version = ".".join(str(x) for x in [majornumber, minornumber])
devbranch = "dev_" + "_".join(str(x) for x in [majornumber, minornumber])

This comment has been minimized.

Copy link
@mtbc

mtbc May 17, 2013

Member

JOOI (as I don't know Python) is there a reason not to do just ...join(map(str, [majornumber, minornumber]))? (of course your code is fine as it is)

This comment has been minimized.

Copy link
@joshmoore

joshmoore May 17, 2013

Member

http://stackoverflow.com/questions/1247486/python-list-comprehension-vs-map etc. etc. I.e. no, not really a reason.

But as long as we're talking about Python style, (majornumber, minornumber) would be preferred over [...,...]): http://stackoverflow.com/questions/1708510/python-list-vs-tuple-when-to-use-each

This comment has been minimized.

Copy link
@mtbc

mtbc May 17, 2013

Member

interesting one, it's homogeneous but immutable (-:

elif majornumber == 4:
return "3.2"
else:
raise "No previous version is defined"

This comment has been minimized.

Copy link
@mtbc

mtbc May 17, 2013

Member

needs an Exception constructor around the string

This comment has been minimized.

Copy link
@bpindelski

bpindelski May 17, 2013

@sbesson Sorry for adding more work, but I'd add a short comment explaining why we hard-code versions in the conf.py file. These things seem to get easily forgotten.

@mtbc

This comment has been minimized.

Copy link
Member

mtbc commented May 17, 2013

I don't know if maintenance would be easier if the previous minor version number were obtained from a file that is basically just the output of openmicroscopy's git tag so that it is always in code that the previous minor version number is determined from among the set of version numbers. But, it's not like your current approach is really all that burdensome either, so really I'm just airing an alternative rather than actually suggesting it.

@sbesson

This comment has been minimized.

Copy link
Member Author

sbesson commented May 17, 2013

@mtbc: I definitely thought of your suggestion. The reason why I chose the hardcoding way in conf.py is because I didn't want the compilation of the documentation to rely upon either a local checkout of the source code or a call to the github API for retrieving the tags.

My feeling is that there is no optimal solution for now. I wonder if the long term solution is not to reduce the level of hardcoding in the db upgrade process itself, i.e. work on a bin/omero db upgrade target that would take care of the server version detection and manage multiple db upgrades (e.g. 4.3 -> 5.0). I know @ctrueden also had suggestions for improving the upgrade process.

@joshmoore, @bpindelski: thoughts about this?

@mtbc

This comment has been minimized.

Copy link
Member

mtbc commented May 17, 2013

@sbesson: db upgrade target: nice idea. The dbpatch table should have enough information.

@bpindelski

This comment has been minimized.

Copy link

bpindelski commented May 17, 2013

@mtbc, @sbesson I also would vote for an automated way of upgrading the DB, so that we can phase out magic strings out of the code.

sbesson added 2 commits May 17, 2013
Also wrap the error in an Exception() when previous version is undefined
@mtbc

This comment has been minimized.

Copy link
Member

mtbc commented May 17, 2013

Great, good to merge.

Your bin/omero db upgrade target is probably worth a ticket.

@sbesson

This comment has been minimized.

Copy link
Member Author

sbesson commented May 17, 2013

@jburel

This comment has been minimized.

Copy link
Member

jburel commented May 19, 2013

Thanks merging.

jburel added a commit that referenced this pull request May 19, 2013
Fix OMERO version number generation
@jburel jburel merged commit bfb29b3 into ome:develop May 19, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.