adoroszlai commented on code in PR #8632: URL: https://github.com/apache/ozone/pull/8632#discussion_r2166271937
########## hadoop-ozone/dist/src/main/smoketest/grpc/grpc-om-s3-metrics.robot: ########## @@ -21,7 +21,8 @@ Library BuiltIn Resource ../commonlib.robot Resource ../s3/commonawslib.robot Test Timeout 5 minutes -Suite Setup Setup s3 tests +Suite Setup Run Keywords Get Security Enabled From Config +... AND Setup s3 tests Review Comment: I think this change is unnecessary. `Setup s3 tests` already calls `Get Security Enabled From Config`: https://github.com/apache/ozone/blob/6d08145fe67b7ca1df0bfc4e674169ba6bc771ce/hadoop-ozone/dist/src/main/smoketest/s3/commonawslib.robot#L133-L137 ########## hadoop-ozone/dist/src/main/smoketest/s3/bucketlist.robot: ########## @@ -20,7 +20,8 @@ Library String Resource ../commonlib.robot Resource commonawslib.robot Test Timeout 5 minutes -Suite Setup Setup s3 tests +Suite Setup Run Keywords Get Security Enabled From Config +... AND Setup s3 tests Review Comment: Unnecessary, done in `Setup s3 tests`. ########## hadoop-ozone/dist/src/main/smoketest/commonlib.robot: ########## @@ -31,12 +31,21 @@ Get test user principal ${instance} = Execute hostname | sed 's/scm[0-9].org/scm/;s/scm[0-9]/scm/;s/om[0-9]/om/' [return] ${user}/${instance}@EXAMPLE.COM +Get Security Enabled From Config + Return From Keyword If '${SECURITY_ENABLED}' != '' + ${value} = Execute ozone getconf confKey ozone.security.enabled + IF '${value}' != 'true' and '${value}' != 'false' + ${value} = Set Variable false + END + Set Global Variable ${SECURITY_ENABLED} ${value} + Kinit HTTP user Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip in unsecure cluster ${principal} = Get test user principal HTTP Wait Until Keyword Succeeds 2min 10sec Execute kinit -k -t /etc/security/keytabs/HTTP.keytab ${principal} Kinit test user + Run Keyword Get Security Enabled From Config Review Comment: `Run Keyword` is useful if the name of the keyword is dynamic. Otherwise it's unnecessary. ```suggestion Get Security Enabled From Config ``` ########## hadoop-ozone/dist/src/main/smoketest/s3/objectputget.robot: ########## @@ -20,7 +20,8 @@ Library String Resource ../commonlib.robot Resource commonawslib.robot Test Timeout 5 minutes -Suite Setup Setup s3 tests +Suite Setup Run Keywords Get Security Enabled From Config +... AND Setup s3 tests Review Comment: Unnecessary, done in `Setup s3 tests`. ########## hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug-ldb.robot: ########## @@ -17,8 +17,10 @@ Documentation Test ozone debug ldb CLI Library OperatingSystem Resource ../lib/os.robot +Resource ../commonlib.robot Test Timeout 5 minute -Suite Setup Write keys +Suite Setup Run Keywords Get Security Enabled From Config +... AND Write keys Review Comment: I think the original `Suite Setup` is fine, just need to remove `${SECURITY_ENABLED}` check here: https://github.com/apache/ozone/blob/6d08145fe67b7ca1df0bfc4e674169ba6bc771ce/hadoop-ozone/dist/src/main/smoketest/debug/ozone-debug-ldb.robot#L33-L34 ########## hadoop-ozone/dist/src/main/smoketest/freon/generate-chunk.robot: ########## @@ -23,4 +23,5 @@ ${PREFIX} ${EMPTY} *** Test Cases *** DN Chunk Generator + Run Keyword Get Security Enabled From Config Review Comment: Can we add it as suite setup for consistency? ########## hadoop-ozone/dist/src/main/smoketest/admincli/container.robot: ########## @@ -26,6 +26,7 @@ ${SCM} scm *** Keywords *** Create test data + Run Keyword Get Security Enabled From Config Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab Review Comment: ```suggestion Kinit test user testuser testuser.keytab ``` ########## hadoop-ozone/dist/src/main/smoketest/certrotation/cert-rotation.robot: ########## @@ -25,6 +25,7 @@ ${port} 9859 *** Keywords *** Setup Test + Run Keyword Get Security Enabled From Config Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab Review Comment: ```suggestion Kinit test user testuser testuser.keytab ``` ########## hadoop-ozone/dist/src/main/smoketest/commonlib.robot: ########## @@ -50,6 +59,6 @@ Access should be denied Requires admin privilege [arguments] ${command} - Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip privilege check in unsecure cluster + Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip privilege check in unsecure cluster Review Comment: nit: let's avoid unnecessary whitespace change ```suggestion Pass Execution If '${SECURITY_ENABLED}' == 'false' Skip privilege check in unsecure cluster ``` ########## hadoop-ozone/dist/src/main/smoketest/basic/links.robot: ########## @@ -20,7 +20,8 @@ Resource ../commonlib.robot Resource ../ozone-lib/shell.robot Test Setup Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit test user testuser testuser.keytab Test Timeout 4 minute -Suite Setup Create volumes +Suite Setup Run Keywords Get Security Enabled From Config +... AND Create volumes Review Comment: Let's move `Get Security Enabled From Config` into `Create volumes`. ########## hadoop-ozone/dist/src/main/smoketest/ozone-lib/freon.robot: ########## @@ -14,11 +14,10 @@ # limitations under the License. *** Settings *** -Resource ../lib/os.robot +Resource ../commonlib.robot Review Comment: This seems to be unnecessary. ########## hadoop-ozone/dist/src/main/smoketest/httpfs/operations.robot: ########## @@ -17,6 +17,7 @@ Library Process Library BuiltIn Library String +Suite Setup Get Security Enabled From Config Review Comment: `operations.robot` does not have test cases (it's a resource file), so this can be removed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
