ome / omero-blitz

Gradle project containing Ice remoting code for OMERO
https://www.openmicroscopy.org/omero
GNU General Public License v2.0
0 stars 15 forks source link

Have reset password use SQL to bypass 2019-SV3 restrictions. #85

Closed mtbc closed 4 years ago

mtbc commented 4 years ago

Use https://github.com/ome/omero-model/pull/51 to fix the https://github.com/ome/openmicroscopy/pull/6230 failure of testPasswordResetEmailGood.

mtbc commented 4 years ago

Last commit to be omitted from merge, can push away once https://github.com/ome/omero-model/pull/51 is in. (Could I use --depends-on within a repo still to get that commit out to another PR?)

mtbc commented 4 years ago

This fixes the integration test.

joshmoore commented 4 years ago

Along with:

the change here looks generally good. I still have a question about the order of operation. Do we want to merge sequentially, rolling back the travis fix. Or merge all the PRs and then use a quick jar release to tie them back together appropriately. Probably worth a quick chat.

mtbc commented 4 years ago

Once we tag omero-model I am happy to force-change that last commit accordingly to see if Travis stays green.

joshmoore commented 4 years ago

Thanks, @mtbc. @ome/release : that implies we would also try to leave the omero-model pinning at the omero-server level to avoid the need for a rendering and romio PR.

sbesson commented 4 years ago

@ome/release : that implies we would also try to leave the omero-model pinning at the omero-server level to avoid the need for a rendering and romio PR.

:+1: for using dependency management to reduce the number of unnecessary intermediate patch releases.

joshmoore commented 4 years ago

Things are looking green. :+1: I'm a bit surprised by the need to have api() and implementation() statements, but barring that, LGTM.

mtbc commented 4 years ago

Is this mergeable or should I further adjust build.gradle somehow?

joshmoore commented 4 years ago

I don't the build.gradle, but I defer to you if there's a need for the duplication.

mtbc commented 4 years ago

There is, it won't build with one of them removed.

joshmoore commented 4 years ago

Testing on merge reaches https://github.com/ome/omero-blitz/blob/67fad3e9a2001de0322fb6e4a06065cc296f2a88/src/main/java/omero/cmd/admin/ResetPasswordRequestI.java#L174 i.e. passes the changes in this PR even though the send does not complete (due to local configuration).

joshmoore commented 4 years ago

Before merging, I pushed a commit matching my understanding of api/implementation. As @jburel points out, "API" should just say "compile and runtime" classpaths, while "implementation" should just be "runtime" class path. If this works out, I'd say good to go.

mtbc commented 4 years ago

Interesting, I wonder if I messed up my local build somehow when I tried. Travis seems happy now at least, yay.

joshmoore commented 4 years ago

:+1: Leaving the set of three PRs in your hand then.

mtbc commented 4 years ago

(You'll have to merge https://github.com/ome/openmicroscopy/pull/6230, I can't.)

mtbc commented 4 years ago

Thank you. :+1: