HUE-8177 [oozie] Add a config check for /user/hue/oozie/workspaces

Review Request #12786 — Created April 5, 2018 and submitted

roohi
hue
HUE-8253_query_dwld_non_ascii
HUE-8177
hue
enricoberti, jgauthier, johan, ranade, romain, weixia, yingc
commit 55dc30eadfaa6556a569685bd13f3b6a45cb8293
Author: Roohi <roohisyeda@cloudera.com>
Date:   Thu Apr 5 15:50:10 2018 -0700

    HUE-8177 [oozie] Add a config check for "/user/hue/oozie/workspaces"

:100644 100644 5112a72ea7... 01b10f64a6... M	apps/oozie/src/oozie/conf.py

Tested manually on chrome

  • 1
  • 0
  • 25
  • 0
  • 26
Description From Last Updated
Removing this is failing the test. roohi roohi
roohi
  1. 
      
  2. apps/oozie/src/oozie/conf.py (Diff revision 1)
     
     

    Romain, you suggested me to add in liboozie, but I have import issues with REMOTE_SAMPLE_DIR so added in oozie app for now. I am not very familiar with importing. Can you pls help me resolve.

  3. 
      
romain
  1. Nice, just a few nits

  2. apps/oozie/src/oozie/conf.py (Diff revision 1)
     
     

    Should work on top as desktop is the core lib?

  3. apps/oozie/src/oozie/conf.py (Diff revision 1)
     
     

    Waht it still a problem when importing like on line 116?

  4. apps/oozie/src/oozie/conf.py (Diff revision 1)
     
     

    If none or does not exist, could we add a warning too?

  5. apps/oozie/src/oozie/conf.py (Diff revision 1)
     
     
    nit:
    NICE_NAME, <space>
    
    nit:
    
    we usually " ... %s  ... are too restrictive" % EMOTE_SAMPLE_DIR.get()
  6. 
      
roohi
romain
  1. 
      
  2. Can't be on top?

  3. Could we move to #82?

  4. desktop/libs/liboozie/src/liboozie/conf.py (Diff revision 2)
     
     
     

    Should we add a live or mock test (this could be tricky)?

    e.g. TestFileBrowserWithHadoop in filebrowser app

    1. I am trying to follow TestFileBrowserWithHadoop. How do I run TestFileBrowserWithHadoop tests?

  5. 
      
romain
  1. 
      
  2. desktop/libs/liboozie/src/liboozie/conf.py (Diff revision 2)
     
     
     

    Live or Mini Cluster
    http://gethue.com/tutorial-how-to-run-the-hue-integration-tests/

    Live should be simpler has no setup is needed, just re-using the current cluster you are pointing too

  3. 
      
roohi
romain
  1. Nice!

    Bunch of comment but those are more about how to make it a bit more efficient

  2. For information, stats can be empty? (no exception raised if the perms or path does not exist?)

  3. desktop/libs/liboozie/src/liboozie/conf.py (Diff revision 3)
     
     
     
     

    If we error if the path does not exist, maybe this two could be combined?

  4. Shouldn't we test only if Oozie is configured?

  5. Could we use 'admin' instead of 'hdfs'?

  6. We should not need this one IIRC

  7. How about setting a

    REMOTE_SAMPLE_DIR.set_for_testing('/path..')

    similarly to

    https://github.com/cloudera/hue/blob/master/apps/beeswax/src/beeswax/tests.py#L836

    so that we are sure that the path does not exist?

  8. nit: no need for new line

  9. and here we do the same, we basically override the default value of REMOTE_SAMPLE_DIR.get() with REMOTE_SAMPLE_DIR.set_for_testing('/....')

  10. We could even chmod it to 777 and then do the check_config call again to validate

  11. 
      
roohi
romain
  1. Really nice, last small tweaks!

  2. Better to do

    reset = REMOTE_SAMPLE_DIR.set_for_testing('/user/hue/oozie/oozie_workspaces_test')

    try:
    ..
    finally:
    reset()

    in both test function, so that the order does not matter at all and we also reset the mocked value?

  3. nit: (usually new line and 4 spaces of indent)

    permissions_dict = {
    'group_read': True, 'other_execute': True,
    ....
    }

  4. 
      
roohi
roohi
  1. 
      
  2. Hi Romain, I was pointing to nightly, and /user/hue/oozie folder is not created until we submit an Oozie job. So creating oozie_workspace_test fails. I think it will not be an issue if a use hdfs user instead of admin. What do you suggest?

    1. You can just point to '/user/hue/oozie_workspaces_test' or even '/tmp/oozie_workspaces_test' as this is just a mock of the path?

  3. 
      
roohi
romain
  1. Nice!

  2. /tmp/test_workspace_exists ?

    That way it does not potentially conflict with the second test?

  3. /tmp/test_workspace_has_enough_permissions

    ?

  4. Maybe move to the finally and remove the assert?

  5. 
      
roohi
roohi
roohi
roohi
romain
  1. Ship It!
  2. 
      
roohi
roohi
  1. 
      
  2. Removing this is failing the test.

  3. 
      
romain
  1. 
      
  2. Could we just check if REMOTE_SAMPLE_DIR.get() exists instead of the exception? (and not go in the if to check the permission if the path does not exist)

  3. 
      
roohi
roohi
Review request changed

Status: Closed (submitted)

Loading...