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

Re-organize edge case upload options for my own understanding. #5206

Merged
merged 1 commit into from Dec 14, 2017

Conversation

Projects
None yet
5 participants
@jmchilton
Copy link
Member

jmchilton commented Dec 13, 2017

I think setting each of these variables once and simplifying the context they are used in (in the case of purge_upload) makes it more clear what each variable is and how it is set. I also think one, more detailed comment for each variable helps.

Note: This will break run-as-user uploads started prior to the upgrade to 18.XX and executed after the upgrade. It is a small switch to restore the old behavior but I'm not sure it is worth the complexity it adds to the file. Added the fix - this shouldn't be a problem for deployers.

Ping @mvdbeek - you are the one who understands these run-as-real-user options the best based on #4539. Does this new organization make sense to you - does it look functionally equivalent to the previous way things were working?

@mvdbeek
Copy link
Member

mvdbeek left a comment

Looks much better, thanks @jmchilton !

@@ -303,7 +317,7 @@ def add_file(dataset, registry, json_file, output_path):
if e.errno != errno.EACCES:
raise
elif link_data_only == 'copy_files':
if purge_source and not run_as_real_user:
if purge_source:

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Dec 13, 2017

Member

The comment below is already addressed with the new comment above purge_source, line 86-87, I think it's better to remove the comment here.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Dec 13, 2017

Also I'd say let's not break uploads that are processed between the transition 17.09 and 18.01 -- I think it's not very likely to be a problem, but as you said the fix is simple. We could add a TODO to remove this past 18.01 ?

in_place = run_as_real_user and dataset.type not in ('server_dir', 'path_paste', 'ftp_import')

# check_content is a flag in Galaxy's config that can be swapped off to disable checking for HTML content
# and such that can prevent uploads from working in some cases.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 13, 2017

Member
    # check_content is a flag in Galaxy's config to enable some security checks
    # on the uploaded content, but can prevent uploads from working in some cases.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 13, 2017

Author Member

I took this as a base and added some more details - thanks!

check_content = dataset.get('check_content' , True)

# auto_decompress is a request flag that can be swapped on to prevent Galaxy from automatically

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Dec 13, 2017

Member

s/on/off/ ?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Dec 13, 2017

Author Member

Yeah - good point. I'll rebase with this fix.

@jmchilton jmchilton force-pushed the jmchilton:upload_option_clarify branch 2 times, most recently from a36c917 to 078fbd4 Dec 13, 2017

Re-organize edge case upload options for my own clarity.
I think setting each of these variables once and simplifing the context they are used in (in the case of purge_upload) makes it more clear what each variable is and how it is set. I also think one, more detailed comment for each variable helps.

Note: This will break run-as-user uploads started prior to the upgrade to 18.XX and executed after the upgrade. It is a small switch to restore the old behavior but I'm not sure it is worth the complexity it adds to the file.

```
run_as_real_user = dataset.get('run_as_real_user', False) or dataset_get.('in_place', True)
```

@jmchilton jmchilton force-pushed the jmchilton:upload_option_clarify branch from 078fbd4 to 099c156 Dec 13, 2017

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Dec 13, 2017

Also I'd say let's not break uploads that are processed between the transition 17.09 and 18.01 -- I think it's not very likely to be a problem, but as you said the fix is simple. We could add a TODO to remove this past 18.01 ?

Rebased to add that! Thanks for confirming this all makes sense @mvdbeek and thanks for comment improvements @nsoranzo.

@dannon

This comment has been minimized.

Copy link
Member

dannon commented Dec 13, 2017

@jmchilton This is good to go, right? This bit of code has always been tough to digest at first glance, this definitely looks like an improvement to me. (Just doublechecking since it doesn't have a review tag)

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Dec 14, 2017

@dannon Yeah - it is ready to go thanks!

@dannon dannon merged commit fce9f13 into galaxyproject:dev Dec 14, 2017

6 checks passed

api test Build finished. 334 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 165 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 60 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 117 tests run, 1 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Jan 1, 2018

This is all wrong...

run_as_real_user=trans.app.config.external_chown_script is None

... doesn't make any sense right? I thought I was finally not confused but I still am it seems.

Update: The previous version of this did the same thing:

run_as_real_user = in_place = dataset.get('in_place', True)

I'm not sure this broke anything that wasn't already broken - but it seems broken to me.

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Jan 1, 2018

Okay - I think the run_as_real_user variable was introduced here 06db294 and was exactly the opposite of what it should be IMO. Otherwise the logic all makes sense to me. That commit isn't in a release I don't think - so I guess no one has tested it with real user uploads (if anyone has those working at all). @mvdbeek does that make sense to you? Should I just swap the run_as_real_user definition in upload_common.py to be the inverse of what it is now?

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Jan 1, 2018

Yes, run_as_real_user is not defined correctly, it should be True if there is an external_chown_script. But I think the definition of in_place may be wrong as well. I think if the upload tool runs as the real user the uploaded file may not be changed by the real user because it is owned by the galaxy user, so in_place should be false. Together those 2 wrong definitions may have actually worked when external_chown_script was set.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Jan 1, 2018

I haven't actually confirmed if that is true though.

@jmchilton

This comment has been minimized.

Copy link
Member Author

jmchilton commented Jan 4, 2018

uploaded file may not be changed by the real user because it is owned by the galaxy user, so in_place should be false

Okay... this makes some sense to me. So this comment I added:

> # In this case we try to reuse the uploaded file that has been chowned to this user already.

was 100% wrong - I should invert the check and the run_as_real_user value and the comment should be:

> # in_place should always be false if running as real user because the uploaded file will be owned by Galaxy and not the user.

I'm opening a PR with this to re-clarify.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Jan 4, 2018

Re-work upload clarification from galaxyproject#5206.
See post-merge discussion on that issue.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Jan 4, 2018

Re-work upload clarification from galaxyproject#5206.
See post-merge discussion on that issue.

martenson added a commit that referenced this pull request Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.