Skip to content
This repository was archived by the owner on Oct 9, 2025. It is now read-only.

Commit 56bf4fc

Browse files
hminaee-tchminaee-tc
committed
IDP-43-modular: feedback review
1 parent f9df219 commit 56bf4fc

File tree

9 files changed

+187
-292
lines changed

9 files changed

+187
-292
lines changed

‎cpk_lib_python_github/github_app_token_generator_package/github_app_token_generator/auth.py‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
"""GitHub App authentication utilities."""
33
import logging
44
import time
5+
from typing import Optional
56

67
import jwt
78

@@ -11,8 +12,8 @@
1112
class GitHubAppAuth:
1213
"""Handle GitHub App authentication."""
1314

14-
def __init__(self, app_id: str, private_key: str):
15-
self.app_id = app_id
15+
def __init__(self, app_id: int, private_key: str):
16+
self.app_id = str(app_id)
1617
self.private_key = private_key
1718

1819
def generate_jwt(self) -> str:
@@ -46,7 +47,10 @@ def read_private_key(private_key_path: str) -> str:
4647
raise error
4748

4849
@staticmethod
49-
def get_private_key_content(private_key_path=None, private_key_content=None) -> str:
50+
def get_private_key_content(
51+
private_key_path: Optional[str] = None,
52+
private_key_content: Optional[str] = None,
53+
) -> str:
5054
"""Get private key content from file or direct input."""
5155
if private_key_path and private_key_content:
5256
raise ValueError("Cannot specify both --private-key-path and --private-key")
@@ -58,8 +62,6 @@ def get_private_key_content(private_key_path=None, private_key_content=None) ->
5862
logger.debug("Using private key content provided directly")
5963
return private_key_content
6064

61-
if private_key_path:
62-
logger.debug("Reading private key from file: %s", private_key_path)
63-
return GitHubAppAuth.read_private_key(private_key_path)
64-
65-
raise ValueError("No private key source provided")
65+
# If we reach here, private_key_path must be truthy
66+
logger.debug("Reading private key from file: %s", private_key_path)
67+
return GitHubAppAuth.read_private_key(private_key_path)

‎cpk_lib_python_github/github_app_token_generator_package/github_app_token_generator/cli.py‎

Lines changed: 25 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,19 @@ def create_parser() -> argparse.ArgumentParser:
1616
epilog=f"""
1717
{Fore.GREEN}{Style.BRIGHT}Examples:{Style.RESET_ALL}
1818
{Fore.CYAN}# Generate tokens (using private key file){Style.RESET_ALL}
19-
{Fore.YELLOW}%(prog)s{Style.RESET_ALL} {Fore.CYAN}--org{Style.RESET_ALL} myorg \\
20-
{Fore.CYAN}--app-id{Style.RESET_ALL} APP_ID \\
21-
{Fore.CYAN}--private-key-path{Style.RESET_ALL} /path/to/key.pem
19+
{Fore.YELLOW}%(prog)s{Style.RESET_ALL} {Fore.CYAN}--app-id{Style.RESET_ALL} $APP_ID \\
20+
{Fore.CYAN}--private-key-path{Style.RESET_ALL} /path/to/key.pem \\
21+
{Fore.CYAN}--org{Style.RESET_ALL} myorg
2222
2323
{Fore.CYAN}# Generate tokens (using private key content){Style.RESET_ALL}
24-
{Fore.YELLOW}%(prog)s{Style.RESET_ALL} {Fore.CYAN}--org{Style.RESET_ALL} myorg \\
25-
{Fore.CYAN}--app-id{Style.RESET_ALL} APP_ID \\
26-
{Fore.CYAN}--private-key{Style.RESET_ALL} "$(cat /path/to/key.pem)"
24+
{Fore.YELLOW}%(prog)s{Style.RESET_ALL} {Fore.CYAN}--app-id{Style.RESET_ALL} $APP_ID \\
25+
{Fore.CYAN}--private-key{Style.RESET_ALL} "$(cat /path/to/key.pem)" \\
26+
{Fore.CYAN}--org{Style.RESET_ALL} myorg
27+
28+
{Fore.CYAN}# Using environment variables{Style.RESET_ALL}
29+
{Fore.YELLOW}export APP_ID=$APP_ID{Style.RESET_ALL}
30+
{Fore.YELLOW}export PRIVATE_KEY_PATH=/path/to/key.pem{Style.RESET_ALL}
31+
{Fore.YELLOW}%(prog)s{Style.RESET_ALL} {Fore.CYAN}--org{Style.RESET_ALL} myorg
2732
2833
{Fore.BLUE}{Style.BRIGHT}Environment Variables:{Style.RESET_ALL}
2934
{Fore.MAGENTA}APP_ID{Style.RESET_ALL} GitHub App ID
@@ -34,40 +39,30 @@ def create_parser() -> argparse.ArgumentParser:
3439

3540
# Add arguments
3641
parser.add_argument(
37-
"--app-id", help="GitHub App ID (or set APP_ID env var)", metavar="ID"
42+
"--app-id", type=int, help="(REQUIRED) GitHub App ID (or set APP_ID env var)", metavar="ID"
3843
)
3944

4045
# Private key group
4146
key_group = parser.add_mutually_exclusive_group()
4247
key_group.add_argument(
43-
"--private-key-path", help="Path to private key file", metavar="PATH"
48+
"--private-key-path",
49+
help="(REQUIRED if private-key not present) Path to private key file",
50+
metavar="PATH",
4451
)
4552
key_group.add_argument(
46-
"--private-key", help="Private key content directly", metavar="KEY_CONTENT"
53+
"--private-key",
54+
help="(REQUIRED if private-key-path not present) Private key content directly",
55+
metavar="KEY_CONTENT",
4756
)
4857

4958
# Operations
50-
parser.add_argument(
51-
"--org", help="Organization name to get token for", metavar="ORG"
52-
)
53-
parser.add_argument(
54-
"--installation-id", help="Installation ID (if known)", metavar="ID"
55-
)
56-
parser.add_argument(
57-
"--list-installations", action="store_true", help="List all installations"
58-
)
59-
parser.add_argument(
60-
"--analyze-app", action="store_true", help="Analyze GitHub App details"
61-
)
62-
parser.add_argument(
63-
"--validate-token", help="Validate an existing token", metavar="TOKEN"
64-
)
65-
parser.add_argument(
66-
"--revoke-token", help="Revoke an existing token", metavar="TOKEN"
67-
)
68-
parser.add_argument(
69-
"--force", action="store_true", help="Skip confirmation prompts"
70-
)
59+
parser.add_argument("--org", help="Organization name to get token for", metavar="ORG")
60+
parser.add_argument("--installation-id", help="Installation ID (if known)", metavar="ID")
61+
parser.add_argument("--list-installations", action="store_true", help="List all installations")
62+
parser.add_argument("--analyze-app", action="store_true", help="Analyze GitHub App details")
63+
parser.add_argument("--validate-token", help="Validate an existing token", metavar="TOKEN")
64+
parser.add_argument("--revoke-token", help="Revoke an existing token", metavar="TOKEN")
65+
parser.add_argument("--force", action="store_true", help="Skip confirmation prompts")
7166
parser.add_argument("--debug", action="store_true", help="Enable debug logging")
7267

7368
return parser

‎cpk_lib_python_github/github_app_token_generator_package/github_app_token_generator/config.py‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,6 @@ class Config:
2020
timeout: int = 30
2121
debug: bool = False
2222

23-
def __post_init__(self):
24-
"""Validate configuration after initialization."""
25-
if self.app_id:
26-
try:
27-
# Ensure app_id is a valid integer
28-
int(self.app_id)
29-
except ValueError as error:
30-
raise ValueError(
31-
f"Invalid app_id: {self.app_id}. Must be a number."
32-
) from error
33-
3423
@property
3524
def has_private_key(self) -> bool:
3625
"""Check if private key is configured."""
@@ -64,9 +53,15 @@ def get_config_from_env(args) -> Config:
6453

6554
# Check if this operation requires app configuration
6655
needs_app_config = requires_app_config(args)
56+
# Handle app_id with proper type conversion
57+
if args.app_id:
58+
config.app_id = args.app_id # Already int from parser
59+
elif os.getenv("APP_ID"):
60+
try:
61+
config.app_id = int(os.getenv("APP_ID")) # Convert env var to int
62+
except ValueError as exc:
63+
raise ValueError(f"Invalid APP_ID environment variable: {os.getenv('APP_ID')}") from exc
6764

68-
# App ID - CLI args take precedence over env vars
69-
config.app_id = args.app_id or os.getenv("APP_ID")
7065
if needs_app_config and not config.app_id:
7166
logger.error(
7267
"App ID is required for this operation. "
@@ -82,7 +77,7 @@ def get_config_from_env(args) -> Config:
8277
config.private_key_content = args.private_key or os.getenv("PRIVATE_KEY")
8378

8479
if needs_app_config and not config.has_private_key:
85-
logger.error(
80+
logger.debug(
8681
"Private key is required for this operation. "
8782
"Set PRIVATE_KEY or PRIVATE_KEY_PATH environment variable "
8883
"or use --private-key/--private-key-path"
@@ -135,9 +130,7 @@ def validate_environment() -> bool:
135130
return False
136131

137132
if not (private_key_path or private_key_content):
138-
logger.warning(
139-
"Neither PRIVATE_KEY_PATH nor PRIVATE_KEY environment variable set"
140-
)
133+
logger.warning("Neither PRIVATE_KEY_PATH nor PRIVATE_KEY environment variable set")
141134
return False
142135

143136
# Validate app_id is a number

‎cpk_lib_python_github/github_app_token_generator_package/github_app_token_generator/formatters.py‎

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -108,17 +108,13 @@ def format_token_validation(self, validation_result: Dict[str, Any]) -> str:
108108

109109
if "repositories_count" in validation_result:
110110
count = validation_result["repositories_count"]
111-
lines.append(
112-
f"Accessible repositories: {Fore.CYAN}{count}{Style.RESET_ALL}"
113-
)
111+
lines.append(f"Accessible repositories: {Fore.CYAN}{count}{Style.RESET_ALL}")
114112

115113
if "rate_limit" in validation_result:
116114
rate_limit = validation_result["rate_limit"]
117115
remaining = rate_limit.get("remaining", "Unknown")
118116
limit = rate_limit.get("limit", "Unknown")
119-
lines.append(
120-
f"Rate limit: {Fore.YELLOW}{remaining}/{limit}{Style.RESET_ALL}"
121-
)
117+
lines.append(f"Rate limit: {Fore.YELLOW}{remaining}/{limit}{Style.RESET_ALL}")
122118

123119
if "scopes" in validation_result and validation_result["scopes"]:
124120
scopes = ", ".join(validation_result["scopes"])
@@ -148,8 +144,7 @@ def _format_basic_app_info(self, app_info: Dict[str, Any]) -> str:
148144
f" Owner: {Fore.MAGENTA}",
149145
f"{owner.get('login', 'Unknown')}{Style.RESET_ALL}",
150146
f" Owner Type: {owner.get('type', 'Unknown')}",
151-
f" URL: {Fore.BLUE}"
152-
f"{app_info.get('html_url', 'Unknown')}{Style.RESET_ALL}",
147+
f" URL: {Fore.BLUE}" f"{app_info.get('html_url', 'Unknown')}{Style.RESET_ALL}",
153148
f" Created: {Fore.BLUE}",
154149
f"{app_info.get('created_at', 'Unknown')}{Style.RESET_ALL}",
155150
"", # Empty line
@@ -177,14 +172,11 @@ def format_app_analysis(
177172
lines.extend(
178173
[
179174
f"{Fore.BLUE}{Style.BRIGHT}📍 Installation Summary{Style.RESET_ALL}",
180-
f" Total Installations: {Fore.YELLOW}",
181-
f"{len(installations)}{Style.RESET_ALL}",
175+
f" Total Installations: {Fore.YELLOW}{len(installations)}{Style.RESET_ALL}",
182176
]
183177
)
184178

185-
total_repos = sum(
186-
repos.get("total_count", 0) for repos in installation_repos.values()
187-
)
179+
total_repos = sum(repos.get("total_count", 0) for repos in installation_repos.values())
188180

189181
lines.extend(
190182
[
@@ -215,11 +207,7 @@ def format_app_analysis(
215207

216208
# Repository details
217209
if installation_repos:
218-
lines.append(
219-
self._format_repo_details(
220-
installations, installation_repos, total_repos
221-
)
222-
)
210+
lines.append(self._format_repo_details(installations, installation_repos, total_repos))
223211

224212
return "\n".join(lines)
225213

@@ -285,9 +273,7 @@ def _format_repo_details(
285273

286274
# Add "more repositories" line if needed
287275
if repo_count > threshold:
288-
lines.append(
289-
f" ... and {repo_count - threshold} more repositories"
290-
)
276+
lines.append(f" ... and {repo_count - threshold} more repositories")
291277

292278
return "\n".join(lines)
293279

‎cpk_lib_python_github/github_app_token_generator_package/github_app_token_generator/github_api.py‎

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,7 @@ class GitHubAPIClient:
1616
def __init__(self, timeout: int = 30):
1717
self.timeout = timeout
1818

19-
def get_installation_access_token(
20-
self, jwt_token: str, installation_id: int
21-
) -> str:
19+
def get_installation_access_token(self, jwt_token: str, installation_id: int) -> str:
2220
"""Get installation access token."""
2321
url = f"{self.BASE_URL}/app/installations/{installation_id}/access_tokens"
2422
headers = {
@@ -27,9 +25,7 @@ def get_installation_access_token(
2725
}
2826

2927
try:
30-
logger.debug(
31-
"Requesting access token for installation ID: %s", installation_id
32-
)
28+
logger.debug("Requesting access token for installation ID: %s", installation_id)
3329
response = requests.post(url, headers=headers, timeout=self.timeout)
3430
response.raise_for_status()
3531

@@ -44,14 +40,10 @@ def get_installation_access_token(
4440
return token_data["token"]
4541

4642
except requests.exceptions.HTTPError as http_error:
47-
logger.error(
48-
"HTTP error occurred while getting access token: %s", http_error
49-
)
43+
logger.error("HTTP error occurred while getting access token: %s", http_error)
5044
raise http_error
5145
except requests.exceptions.RequestException as req_error:
52-
logger.error(
53-
"Request error occurred while getting access token: %s", req_error
54-
)
46+
logger.error("Request error occurred while getting access token: %s", req_error)
5547
raise req_error
5648

5749
def list_installations(self, jwt_token: str) -> List[Dict[str, Any]]:
@@ -72,14 +64,10 @@ def list_installations(self, jwt_token: str) -> List[Dict[str, Any]]:
7264
return installations
7365

7466
except requests.exceptions.HTTPError as http_error:
75-
logger.error(
76-
"HTTP error occurred while listing installations: %s", http_error
77-
)
67+
logger.error("HTTP error occurred while listing installations: %s", http_error)
7868
raise http_error
7969
except requests.exceptions.RequestException as req_error:
80-
logger.error(
81-
"Request error occurred while listing installations: %s", req_error
82-
)
70+
logger.error("Request error occurred while listing installations: %s", req_error)
8371
raise req_error
8472

8573
def validate_token(self, token: str) -> Dict[str, Any]:
@@ -122,9 +110,7 @@ def validate_token(self, token: str) -> Dict[str, Any]:
122110
return {
123111
"valid": False,
124112
"status_code": response.status_code,
125-
"reason": error_messages.get(
126-
response.status_code, f"HTTP {response.status_code}"
127-
),
113+
"reason": error_messages.get(response.status_code, f"HTTP {response.status_code}"),
128114
}
129115

130116
except requests.exceptions.RequestException as req_error:
@@ -148,10 +134,7 @@ def revoke_installation_token(self, token: str) -> bool:
148134
return True
149135

150136
except requests.exceptions.HTTPError as http_error:
151-
if (
152-
http_error.response is not None
153-
and http_error.response.status_code == 404
154-
):
137+
if http_error.response is not None and http_error.response.status_code == 404:
155138
logger.warning("Token not found or already revoked")
156139
return False
157140
logger.error("HTTP error occurred while revoking token: %s", http_error)
@@ -181,15 +164,11 @@ def get_app_info(self, jwt_token: str) -> Dict[str, Any]:
181164
logger.error("Error fetching app info: %s", error)
182165
raise error
183166

184-
def get_installation_repositories(
185-
self, jwt_token: str, installation_id: int
186-
) -> Dict[str, Any]:
167+
def get_installation_repositories(self, jwt_token: str, installation_id: int) -> Dict[str, Any]:
187168
"""Get repositories accessible by installation."""
188169
try:
189170
# First get an installation access token
190-
installation_token = self.get_installation_access_token(
191-
jwt_token, installation_id
192-
)
171+
installation_token = self.get_installation_access_token(jwt_token, installation_id)
193172

194173
# Use the installation token to get repositories
195174
url = f"{self.BASE_URL}/installation/repositories"
@@ -209,9 +188,7 @@ def get_installation_repositories(
209188
# Return empty result instead of raising
210189
return {"total_count": 0, "repositories": [], "error": str(error)}
211190

212-
def get_accessible_repositories_via_token(
213-
self, installation_token: str
214-
) -> Dict[str, Any]:
191+
def get_accessible_repositories_via_token(self, installation_token: str) -> Dict[str, Any]:
215192
"""Get repositories using installation token instead of JWT."""
216193
url = f"{self.BASE_URL}/installation/repositories"
217194
headers = {

0 commit comments

Comments
 (0)