-
Notifications
You must be signed in to change notification settings - Fork 100
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
11591 user ip #1742
11591 user ip #1742
Conversation
@aleksandra-tarkowska : this requires a purging the DB on gretzky. I'm marking excluded for the moment. If @pwalczysko is ok with deleting all the data on gretzky for Monday, than I can unexclude. Otherwise, we can discuss Monday AM. Thanks! |
We will need to check with @imunro and @melissalinkert if there is still some on-going modulo work too. |
If all this means that you want to clear Gretzky then feel free to go ahead & delete anything I've got on there. |
Fine with clearing Gretzky. Please double check with @melissalinkert . If Purge Data option here http://hudson.openmicroscopy.org.uk/job/OMERO-merge-develop/configure is used, I suppose the auto_import script will kick in - this is how it was before and do not think it changed. Would prefer to leave it so = have always some data on Gretzky, but of course change it if this is necessary for this PR. |
Keep excluded until at least November 20th (demo in Aberdeen). |
Perhaps now include and test? |
@@ -1866,6 +1866,7 @@ | |||
timeToIdle int8 not null, | |||
timeToLive int8 not null, | |||
userAgent varchar(255), | |||
userIP varchar(255), |
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.
Are you expecting host names as well as IPs? If not, 15 digits would suffice. It would have to be a later SQL statement to shrink the column, since we don't support specifying string widths.
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.
I only expect IP. True will short it.
Perhaps now include and test? |
I certainly have no objection (not sure why I'm cc'd actually) |
There are several things that will need to happen here before unexcluding:
Though we can proceed with those done, there should also be a general discussion of the target and requirements for this. What's the plan for other clients? Etc. |
Aha, thanks. For my work on import logs, etc., obviously I'll also have to adjust the DB schema, so if that's to go into 5.0.0, hopefully at the end of this month, then my question would be if I should be expecting to rebase against this merged work first, or I'll probably just introduce more conflicts that further block this PR. |
@@ -2071,6 +2071,11 @@ create unique index originalfile_repo_path_index on originalfile (repo, path, na | |||
-- end ticket:2201 | |||
-- | |||
|
|||
|
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.
This change will need to also be added to the template files under components/dsl/resources/...
There should be no conflicts, after changing schema, two commits https://github.com/aleksandra-tarkowska/openmicroscopy/commit/38808bea92bf93bf126e19bbf597425e09746c29 & https://github.com/aleksandra-tarkowska/openmicroscopy/commit/4221db65690c7b01333b79d74a9a0cb62dde719b were replaced by https://github.com/aleksandra-tarkowska/openmicroscopy/commit/27e0a2728bce284d9bfc443834c6d84feecad7a6 |
@aleksandra-tarkowska: insight already check the IP so it will be an easy addition if we go ahead. |
@@ -2071,6 +2071,11 @@ create unique index originalfile_repo_path_index on originalfile (repo, path, na | |||
-- end ticket:2201 | |||
-- | |||
|
|||
|
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.
This change will need to also make it into dsl/resources/**footer.vm
Sorry, @aleksandra-tarkowska : but this is still listed as "We can’t automatically merge this pull request." |
@joshmoore is it better now? |
PR is mergeable and footer is updated. We'll still have to schedule a re-install on trout. |
@pwalczysko how do you feel about db re-install? |
We are talking trout here (=develop). As discussed with @aleksandra-tarkowska ,
|
Unexcluding. |
this include get_client_ip function reading IP address from HTTP request
@aleksandra-tarkowska: thanks. Last force push was due to the 5.0.0 sql scripts being moved rather than created. Restarted http://ci.openmicroscopy.org/view/Breaking/job/OMERO-5.1-breaking-trigger/ to test this change. Also created http://ci.openmicroscopy.org/view/Breaking/job/OMERO-5.1-breaking-upgrade/ to test the upgrade from a 5.0.0 DB for now. |
When that PR was opened there were next patch for 5.0DEV created following db schema instruction. We do not have anything mentioned in the doc how to create deal with first schema for new version |
ALl breaking builds are green including http://ci.openmicroscopy.org/view/Breaking/job/OMERO-5.1-breaking-upgrade/12/. Altered the merge DB
|
Thanks everyone for the thorough review. |
thank you ;-) |
Ran the upgrade script on the latest DB
|
--no-rebase |
It looks like server can get the IP address of the caller, but if the client is connected to the server via Glacier2, what is reported as the remote address is the Glacier2 endpoint that forwards the request, not the original endpoint of the client. See http://doc.zeroc.com/pages/viewpage.action?pageId=2523170
|
This PR allow to store IP address of the client creating omero session.
It should be tested from all our language bidings.
For example PYTHON:
All integration tests should pass as defined here. The webclient does not yet make use of these changes.