Copilot commented on code in PR #547:
URL: https://github.com/apache/ranger/pull/547#discussion_r2114809295


##########
PyTest-KMS-HDFS/test_kms/utils.py:
##########
@@ -0,0 +1,14 @@
+#no scope mismatch due to utils

Review Comment:
   [nitpick] This comment is unclear—consider clarifying the intent or removing 
it if it isn't adding meaningful context.



##########
test-pytest.sh:
##########
@@ -0,0 +1,110 @@
+#!/bin/bash
+
+#handle input
+if [ $# -lt 1 ]; then
+  echo "no arguments passed , using default DB: postgres"
+  DB_TYPE=postgres
+  EXTRA_SERVICES=()
+
+else
+  DB_TYPE=$1
+  shift
+  EXTRA_SERVICES=("$@")
+fi
+
+# Remove all containers and clean up docker space
+docker rm -f $(docker ps -aq --filter "name=ranger")
+docker system prune --all --force --volumes
+
+#path setup
+RANGER_DOCKER_PATH="dev-support/ranger-docker"
+TESTS_PATH="$HOME/Desktop/PyTest-KMS-EP"

Review Comment:
   The `TESTS_PATH` points to `PyTest-KMS-EP` but the test directory is 
`PyTest-KMS-HDFS`; update the path to the correct test folder.
   ```suggestion
   TESTS_PATH="$HOME/Desktop/PyTest-KMS-HDFS"
   ```



##########
PyTest-KMS-HDFS/test_kms/test_blacklisting.py:
##########
@@ -0,0 +1,263 @@
+import xml.etree.ElementTree as ET
+import requests
+import pytest
+import time
+import docker
+import tarfile
+import io
+
+DBKS_SITE_PATH = 
"/opt/ranger/ranger-3.0.0-SNAPSHOT-kms/ews/webapp/WEB-INF/classes/conf/dbks-site.xml"
+KMS_SERVICE_NAME = "dev_kms"
+BASE_URL = "http://localhost:9292/kms/v1";   
+RANGER_AUTH = ('keyadmin', 'rangerR0cks!')  # Ranger key admin user
+BASE_URL_RANGER = "http://localhost:6080/service/public/v2/api/policy";
+HEADERS={"Content-Type": "application/json","Accept":"application/json"}
+KMS_CONTAINER_NAME = "ranger-kms"
+
+user1="nobody"
+
+client = docker.from_env()
+container = client.containers.get(KMS_CONTAINER_NAME)
+
+
+# **************** create KMS policy for user1 
--------------------------------------
+@pytest.fixture(scope="module", autouse=True)
+def create_initial_kms_policy():
+    policy_data = {
+        "policyName": "blacklist-policy",
+        "service": KMS_SERVICE_NAME,
+        "resources": {
+            "keyname": {
+                "values": ["blacklist-*"],  # Match any key starting with 
'blacklist-'
+                "isExcludes": False,
+                "isRecursive": False
+            }
+        },
+        "policyItems": [{
+            "accesses": [
+                {"type": "CREATE", "isAllowed": True},
+                {"type": "ROLLOVER", "isAllowed": True},
+                {"type": "DELETE", "isAllowed": True}
+            ],
+            "users": [user1]
+        }]
+    }
+
+    response = requests.post(BASE_URL_RANGER, auth=RANGER_AUTH, 
json=policy_data)
+    time.sleep(30)  # Wait for policy propagation

Review Comment:
   Fixed `sleep` delays can slow down tests and add flakiness; consider polling 
the service for readiness instead of a blind 30s wait.



##########
PyTest-KMS-HDFS/test_hdfs/test_config.py:
##########
@@ -0,0 +1,79 @@
+
+##Contains all constant values regarding USER, PATH, HDFS 
Commands----------------------
+
+
+HDFS_USER = "hdfs"
+HIVE_USER = "hive"
+HBASE_USER= "hbase"
+KEY_ADMIN="keyadmin"
+HEADERS={"Content-Type": "application/json","Accept":"application/json"}
+PARAMS={"user.name":"keyadmin"}
+BASE_URL="http://localhost:9292/kms/v1";
+HADOOP_CONTAINER = "ranger-hadoop"
+HDFS_USER = "hdfs"

Review Comment:
   Duplicate definition of `HDFS_USER`; remove the redundant assignment to 
reduce confusion.
   ```suggestion
   # Line 13 removed as it is redundant.
   ```



##########
PyTest-KMS-HDFS/test_kms/utils.py:
##########
@@ -0,0 +1,14 @@
+#no scope mismatch due to utils
+import subprocess
+
+KMS_CONTAINER_NAME = "ranger-kms"             # Replace with your actual KMS 
container name
+KMS_LOG_FILE = "/var/log/ranger/kms/ranger-kms-ranger-kms.example.com-root.log"
+
+def fetch_logs():
+    try:
+        cmd = f"docker exec {KMS_CONTAINER_NAME} tail -n 100 {KMS_LOG_FILE}"
+        logs = subprocess.check_output(cmd, shell=True, text=True)
+        error_logs = [line for line in logs.split("\n") if "ERROR" in line or 
"Exception" in line]
+        return "\n".join(error_logs) if error_logs else "No recent errors in 
logs."
+    except Exception as e:
+        return f"Failed to fetch logs from container: {str(e)}"

Review Comment:
   [nitpick] Catching all exceptions can mask specific errors; consider 
catching only the exceptions you expect (e.g., `subprocess.CalledProcessError`).
   ```suggestion
       except subprocess.CalledProcessError as e:
           return f"Failed to fetch logs from container. Command failed with 
exit code {e.returncode}: {e.output}"
   ```



-- 
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: dev-unsubscr...@ranger.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to