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

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

Roohi Syeda
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 Syeda
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  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. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. Removing this is failing the test.

  3. 
      
  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. 
      
Review request changed

Status: Closed (submitted)

Loading...