Skip to content

Commit 1e73d78

Browse files
MK8S-25: Address PR review comments - refactor and improve code quality
Address all review feedback from rdebay-scality: 1. Create INDEX_HTML_FILENAME constant to avoid typos in filename strings 2. Add _get_saltenv_directories() helper method to factorize repeated saltenv directory path creation logic 3. Improve comment clarity: "Actually remove the index.html files" 4. Use constant and helper method throughout clean/create/targets functions Code improvements: - Eliminate code duplication for saltenv path creation - Single source of truth for index.html filename - Cleaner, more maintainable code structure - Better error prevention with constants instead of magic strings
1 parent 41a4334 commit 1e73d78

File tree

1 file changed

+48
-48
lines changed

1 file changed

+48
-48
lines changed

buildchain/buildchain/targets/repository.py

Lines changed: 48 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ class RPMRepository(Repository):
143143
"""A software repository for CentOS x86_64."""
144144

145145
ARCH = "x86_64"
146+
INDEX_HTML_FILENAME = "index.html"
146147

147148
def __init__(
148149
self,
@@ -207,40 +208,55 @@ def clean() -> None:
207208
task["file_dep"].extend([self.get_rpm_path(pkg) for pkg in self.packages])
208209
return task
209210

211+
def _get_saltenv_directories(self) -> tuple:
212+
"""Get saltenv directory paths for scality repository.
213+
214+
Returns:
215+
Tuple of (saltenv_dir, top_saltenv_dir) paths
216+
"""
217+
if self.name != "scality":
218+
return None, None
219+
220+
# Saltenv directory at repository level (cleaner for cleanup)
221+
saltenv_dir = (
222+
self._repo_root
223+
/ self._releasever
224+
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
225+
)
226+
# Top-level saltenv directory (needed for health check)
227+
top_saltenv_dir = (
228+
self._repo_root / f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
229+
)
230+
return saltenv_dir, top_saltenv_dir
231+
210232
def create_index_files(self) -> types.TaskDict:
211233
"""Create index.html files for repository directories."""
212234

213235
def clean() -> None:
214236
"""Delete the index.html files created by this task."""
215237
# Clean repository-specific index files
216238
index_files = [
217-
self.rootdir / "index.html",
218-
self.rootdir / self.ARCH / "index.html",
239+
self.rootdir / self.INDEX_HTML_FILENAME,
240+
self.rootdir / self.ARCH / self.INDEX_HTML_FILENAME,
219241
]
220242
# Clean shared directory index files if this is scality repo
221-
if self.name == "scality":
222-
# Saltenv directory at repository level (cleaner for cleanup)
223-
saltenv_dir = (
224-
self._repo_root
225-
/ self._releasever
226-
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
227-
)
228-
# Top-level saltenv directory (needed for health check)
229-
top_saltenv_dir = (
230-
self._repo_root
231-
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
232-
)
243+
saltenv_dir, top_saltenv_dir = self._get_saltenv_directories()
244+
if saltenv_dir and top_saltenv_dir:
233245
index_files.extend(
234246
[
235247
# Repository-level saltenv structure
236-
saltenv_dir / "index.html",
237-
saltenv_dir / "redhat" / "index.html",
238-
saltenv_dir / "redhat" / str(self._releasever) / "index.html",
248+
saltenv_dir / self.INDEX_HTML_FILENAME,
249+
saltenv_dir / "redhat" / self.INDEX_HTML_FILENAME,
250+
saltenv_dir
251+
/ "redhat"
252+
/ str(self._releasever)
253+
/ self.INDEX_HTML_FILENAME,
239254
# Top-level saltenv (for health check)
240-
top_saltenv_dir / "index.html",
255+
top_saltenv_dir / self.INDEX_HTML_FILENAME,
241256
]
242257
)
243258

259+
# Actually remove the index.html files
244260
for index_file in index_files:
245261
if index_file.exists():
246262
index_file.unlink()
@@ -254,18 +270,8 @@ def create_index() -> None:
254270

255271
# Also create shared directory index files, but only from scality repo
256272
# to avoid doit task conflicts between multiple repositories
257-
if self.name == "scality":
258-
# Saltenv directory at repository level (cleaner for cleanup)
259-
saltenv_dir = (
260-
self._repo_root
261-
/ self._releasever
262-
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
263-
)
264-
# Top-level saltenv directory (needed for health check)
265-
top_saltenv_dir = (
266-
self._repo_root
267-
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
268-
)
273+
saltenv_dir, top_saltenv_dir = self._get_saltenv_directories()
274+
if saltenv_dir and top_saltenv_dir:
269275
repository_directories.extend(
270276
[
271277
# Repository-level saltenv structure
@@ -280,35 +286,29 @@ def create_index() -> None:
280286
for repository_directory in repository_directories:
281287
# Create directory if it doesn't exist, then create index.html
282288
repository_directory.mkdir(parents=True, exist_ok=True)
283-
index_file = repository_directory / "index.html"
289+
index_file = repository_directory / self.INDEX_HTML_FILENAME
284290
index_file.touch()
285291
# Set readable permissions for nginx
286292
index_file.chmod(0o644)
287293

288294
# Build targets list - include shared directories only for scality repo
289295
targets = [
290-
self.rootdir / "index.html",
291-
self.rootdir / self.ARCH / "index.html",
296+
self.rootdir / self.INDEX_HTML_FILENAME,
297+
self.rootdir / self.ARCH / self.INDEX_HTML_FILENAME,
292298
]
293-
if self.name == "scality":
294-
# Saltenv directory at repository level (cleaner for cleanup)
295-
saltenv_dir = (
296-
self._repo_root
297-
/ self._releasever
298-
/ f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
299-
)
300-
# Top-level saltenv directory (needed for health check)
301-
top_saltenv_dir = (
302-
self._repo_root / f"{config.PROJECT_NAME.lower()}-{versions.VERSION}"
303-
)
299+
saltenv_dir, top_saltenv_dir = self._get_saltenv_directories()
300+
if saltenv_dir and top_saltenv_dir:
304301
targets.extend(
305302
[
306303
# Repository-level saltenv structure
307-
saltenv_dir / "index.html",
308-
saltenv_dir / "redhat" / "index.html",
309-
saltenv_dir / "redhat" / str(self._releasever) / "index.html",
304+
saltenv_dir / self.INDEX_HTML_FILENAME,
305+
saltenv_dir / "redhat" / self.INDEX_HTML_FILENAME,
306+
saltenv_dir
307+
/ "redhat"
308+
/ str(self._releasever)
309+
/ self.INDEX_HTML_FILENAME,
310310
# Top-level saltenv (for health check)
311-
top_saltenv_dir / "index.html",
311+
top_saltenv_dir / self.INDEX_HTML_FILENAME,
312312
]
313313
)
314314

0 commit comments

Comments
 (0)