HUE-9455 [filebrowser] File system user home directory is wrong in S3 only configuration

Review Request #15403 — Created Aug. 27, 2020 and submitted

ayush.goyal
hue
master
HUE-9455
hue
Amlesh1902, johan, ranade, romain, Sreenath, yingc
commit bdef2458e16f763710197234153364f7015c35fe
Author: agl29 <ayushkr.goyal.eee15@itbhu.ac.in>
Date:   Thu Aug 27 19:01:14 2020 +0530

    HUE-9455 File system user home directory is wrong in S3 only configuration

:100644 100644 67a9b47fa2 79945e7402 M	apps/filebrowser/src/filebrowser/conf.py
:100644 100644 17e2ef3b52 b4f72981db M	desktop/core/src/desktop/models.py


  • 12
  • 0
  • 0
  • 0
  • 12
Description From Last Updated
Not sure which conf this is pointing to but you can better add on top: from filebrowser.conf import REMOTE_STORAGE_HOME and ... johan johan
Here this is the home directory aka on the home lage that contains the user documents (/home), so this is ... romain romain
nit, add an example? (also to update in both inis): Optionally set this if you want a different home directory ... romain romain
If it gets too long, feel free to add a variable each time? e.g. home_path = (REMOTE_STORAGE_HOME.get() if REMOTE_STORAGE_HOME.get() else ... romain romain
ditto romain romain
remote_storage_home=s3a://gethue ? romain romain
import to remove too romain romain
Wouldn't it be better to move the logic into `get_home_directory()` instead? romain romain
Could you also add something similar to https://github.com/cloudera/hue/blob/master/desktop/core/src/desktop/templates/global_js_constants.mako#L121 REMOTE_STORAGE_HOME.get() if hasattr(REMOTE_STORAGE_HOME, 'get') and REMOTE_STORAGE_HOME.get() else user.get_home_directory() to avoid the problem ... romain romain
For the record, if it repeats a lot, better to have the setting name = None, e.g. https://github.com/cloudera/hue/blob/master/desktop/libs/notebook/src/notebook/connectors/hiveserver2.py#L68 Even better, ... romain romain
and then: home_path = remote_home_storage if remote_home_storage else self.user.get_home_directory().encode('utf-8') .... romain romain
Move to line 1918? (surrounded by new lines) (Thay way it is scoped only in the function, not global) romain romain
johan
  1. 
      
  2. desktop/core/src/desktop/models.py (Diff revision 1)
     
     

    Not sure which conf this is pointing to but you can better add on top:

    from filebrowser.conf import REMOTE_STORAGE_HOME

    and then:

    if REMOTE_STORAGE_HOME.get():

  3. 
      
ayush.goyal
ayush.goyal
romain
  1. 
      
  2. desktop/core/src/desktop/models.py (Diff revision 2)
     
     

    Here this is the home directory aka on the home lage that contains the user documents (/home), so this is unrelated to File Browser. Could you update the specific storage filesystem home dir in File Browser instead?

    cf. the jira, the idea is to have for example a default landing S3 bucket
    And also fix the refresh icon of the left assist file browser

  3. 
      
ayush.goyal
romain
  1. Nice!

    And any chance to add a small test?

  2. nit, add an example? (also to update in both inis):

    Optionally set this if you want a different home directory path. e.g. s3a://gethue

  3. desktop/core/src/desktop/models.py (Diff revision 3)
     
     

    If it gets too long, feel free to add a variable each time?

    e.g.

    home_path = (REMOTE_STORAGE_HOME.get() if REMOTE_STORAGE_HOME.get() else 'S3A://').encode('utf-8')

    Also note the extra encoding. Does it work when specifying a bucket (and having only S3 configured?)

    remote_storage_home=s3a://demo-gethue

    1. yes, its works, but it not automatically creates bucket

  4. 
      
ayush.goyal
romain
  1. 
      
  2. desktop/conf.dist/hue.ini (Diff revision 4)
     
     
  3. remote_storage_home=s3a://gethue

    ?

  4. 
      
ayush.goyal
ayush.goyal
romain
  1. Ship It!
  2. 
      
ayush.goyal
ayush.goyal
ayush.goyal
romain
  1. 
      
  2. Wouldn't it be better to move the logic into `get_home_directory()` instead?
    1. We can add there also but for this i have to change in this file https://github.com/cloudera/hue/blob/master/desktop/core/src/desktop/auth/backend.py#L168. If it is okay, i can add below line in backend.py and delete from .mako.

      return REMOTE_STORAGE_HOME.get() if hasattr(REMOTE_STORAGE_HOME, 'get') and REMOTE_STORAGE_HOME.get() else self._get_profile().home_directory.

  3. 
      
romain
  1. 
      
  2. Could you also add something similar to

    https://github.com/cloudera/hue/blob/master/desktop/core/src/desktop/templates/global_js_constants.mako#L121

    REMOTE_STORAGE_HOME.get() if hasattr(REMOTE_STORAGE_HOME, 'get') and REMOTE_STORAGE_HOME.get() else user.get_home_directory()

    to avoid the problem when the app is blacklister

    1. Okay, done(at other places also)

  3. 
      
romain
  1. 
      
  2. apps/useradmin/src/useradmin/models.py (Diff revision 7)
     
     

    import to remove too

  3. 
      
ayush.goyal
romain
  1. Notes:
    - Please use a fresh review next time (after the first commit is pushed, we should close the review and use fresh ones for the follow-ups, it is much clearer)
    - To test it with file browser out, in the [desktop] section of the ini, app_blacklist=filebrowser:
    
    e.g.
    
    [desktop]
    app_blacklist=filebrowser
      1. Okay, i will remember that
      2. yes, after app_blacklist=filebrowser hue is working
  2. desktop/core/src/desktop/models.py (Diff revision 8)
     
     

    For the record, if it repeats a lot, better to have the setting name = None, e.g. https://github.com/cloudera/hue/blob/master/desktop/libs/notebook/src/notebook/connectors/hiveserver2.py#L68

    Even better, maybe just extrac the value once on top:

    remote_home_storage =
    REMOTE_STORAGE_HOME.get() if hasattr(REMOTE_STORAGE_HOME, 'get') and REMOTE_STORAGE_HOME.get() else None

  3. desktop/core/src/desktop/models.py (Diff revision 8)
     
     

    and then:

    home_path = remote_home_storage if remote_home_storage else self.user.get_home_directory().encode('utf-8')

    ....

  4. desktop/core/src/desktop/models.py (Diff revision 8)
     
     

    Note (for in the future): at some point we should have a get_home() for each fs)

  5. 
      
ayush.goyal
romain
  1. 
      
  2. desktop/core/src/desktop/models.py (Diff revision 9)
     
     

    Move to line 1918? (surrounded by new lines)
    (Thay way it is scoped only in the function, not global)

  3. 
      
ayush.goyal
ayush.goyal
Review request changed

Status: Closed (submitted)

Loading...