fix: resolve warnings and improve file extraction logic #773

ShigrafS · 2025-07-27T09:58:48Z

Closes: #755

Description

This draft PR addresses and aims to fix all existing errors, warnings, and lint issues throughout the codebase to improve overall code quality, stability, and maintainability. It includes fixes in utilities, sanitization, and other core packages with the following key changes:

Fixed improper resource handling such as missing or misused defer statements (e.g., closing files and readers).

This PR serves as a foundation for a cleaner, more maintainable codebase by systematically resolving technical debt and improving developer experience.

Notes for Reviewers

This is a draft PR and may require iterative reviews and tests.
Some changes are large refactors; please pay close attention to file handling and error wrapping.
Unit tests and integration tests should be run to ensure no regressions.
Feedback on potential edge cases or improvements in error messaging is welcome.
Additional lint rules might be applied progressively in subsequent PRs.

[Signed commits](../CONTRIBUTING.md#signing-off-on-commits-developer-certificate-of-origin)

Yes, I signed my commits.

welcome · 2025-07-27T09:58:50Z

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while performing a commit.

gemini-code-assist

Summary of Changes

Hello @ShigrafS, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the codebase's stability and maintainability by systematically resolving existing warnings and linting issues. A primary aspect of these changes involves refining resource management, particularly ensuring that file and I/O operations are properly closed with robust error handling. Additionally, it introduces new CI tooling to streamline the debugging of linter checks.

Highlights

Code Quality & Linting: Addressed various linting issues and warnings across the codebase to improve overall code quality and maintainability, as indicated by the PR title and description.
Robust Resource Handling: Significantly improved resource handling by ensuring proper error-checked defer statements for closing files, readers, and writers across multiple utility and file processing functions, preventing potential resource leaks and improving stability.
Build & CI Improvements: Introduced a new GitHub Actions workflow (.github/debug-build.yml) to facilitate debugging linter runs on push events, and updated the Makefile to allow passing extra arguments to golangci-lint for more flexible execution.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request resolves warnings and improves file extraction logic by addressing unhandled errors from Close() methods on various resources. The changes correctly identify where error handling was missing, particularly with defer statements.

However, in several utility functions within utils/utils.go, logging the error from Close() is insufficient. For functions that write data to files or streams (like WriteToFile, WriteYamlToFile, Compress), errors on Close() can indicate critical failures (e.g., failure to flush buffered data) and should be propagated to the caller instead of being silently logged. My review focuses on these areas, suggesting patterns to correctly return these errors.

Additionally, I've provided a suggestion to optimize the new GitHub Actions workflow to be more resource-efficient.

gemini-code-assist · 2025-07-27T10:01:36Z

utils/utils.go

 	if err != nil {
 		return ErrCreateFile(err, path)
 	}
+	defer func() {
+		if cerr := file.Close(); cerr != nil {
+			log.Printf("error closing file %s: %v", path, cerr)
+		}
+	}()

 	_, err = file.WriteString(content)
 	if err != nil {
 		return ErrWriteFile(err, path)
 	}
-	// Close the file to save the changes.
-	err = file.Close()
-	if err != nil {
-		return ErrWriteFile(err, path)
-	}
+
 	return nil
 }


The defer statement logs the error from file.Close(), but this can hide critical issues like a failed disk write on flush. The original code returned this error, and this behavior should be preserved. A caller of WriteToFile needs to know if the write was successful, including the file being closed correctly.

To ensure the error is returned, use a named return value for the error and update it in the deferred function. This requires changing the function signature to func WriteToFile(path string, content string) (err error) and then modifying the defer block to assign to err.

func WriteToFile(path string, content string) (err error) { file, err := os.Create(path) if err != nil { return ErrCreateFile(err, path) } defer func() { if cerr := file.Close(); cerr != nil && err == nil { err = cerr log.Printf("error closing file %s: %v", path, cerr) } }() _, err = file.WriteString(content) if err != nil { return ErrWriteFile(err, path) } return nil }

gemini-code-assist · 2025-07-27T10:01:36Z

utils/utils.go

 	zr := gzip.NewWriter(buf)
-	defer zr.Close()
+	defer func() {
+		if cerr := zr.Close(); cerr != nil {
+			log.Printf("error closing gzip writer: %v", cerr)
+		}
+	}()
 	tw := tar.NewWriter(zr)
-	defer tw.Close()
+	defer func() {
+		if cerr := tw.Close(); cerr != nil {
+			log.Printf("error closing tar writer: %v", cerr)
+		}
+	}()


The errors from zr.Close() and tw.Close() are critical because these writers buffer data. A failure on Close often means a failure to flush data, resulting in a corrupt or incomplete archive. These errors are currently only logged and should be returned by the Compress function.

Use a named return value for the error and update it in the deferred functions. Since defer executes in LIFO order, tw.Close() will be called first, then zr.Close(). The error handling logic should ensure the first non-nil error is returned.

func Compress(src string, buf io.Writer) (err error) { zr := gzip.NewWriter(buf) defer func() { if cerr := zr.Close(); cerr != nil && err == nil { err = cerr log.Printf("error closing gzip writer: %v", cerr) } }() tw := tar.NewWriter(zr) defer func() { if cerr := tw.Close(); cerr != nil && err == nil { err = cerr log.Printf("error closing tar writer: %v", cerr) } }() return filepath.Walk(src, func(file string, fi os.FileInfo, walkErr error) error { if walkErr != nil { return walkErr } header, err := tar.FileInfoHeader(fi, file) if err != nil { return err } relPath, err := filepath.Rel(src, file) if err != nil { return err } header.Name = filepath.ToSlash(relPath) if err := tw.WriteHeader(header); err != nil { return err } if !fi.IsDir() { data, err := os.Open(file) if err != nil { return err } defer func() { if cerr := data.Close(); cerr != nil { log.Printf("error closing file %s: %v", file, cerr) } }() _, err = io.Copy(tw, data) if err != nil { return err } } return nil }) }

gemini-code-assist · 2025-07-27T10:01:36Z

utils/utils.go

+			defer func() {
+				if cerr := data.Close(); cerr != nil {
+					log.Printf("error closing file %s: %v", file, cerr)
+				}
+			}()


The error from data.Close() is only logged inside this filepath.Walk callback. An error during file close should be propagated up by returning it from this anonymous function. This will stop the filepath.Walk and signal a failure in the compression process.

Consider not using defer here and instead closing the file explicitly after io.Copy, checking and returning any errors from both operations.

if data, err := os.Open(file); err == nil { _, err = io.Copy(tw, data) if err != nil { data.Close() // ignore error return err } err = data.Close() if err != nil { log.Printf("error closing file %s: %v", file, err) return err } } else { return err }

gemini-code-assist · 2025-07-27T10:01:36Z

.github/debug-build.yml

+on:
+  push:
+    branches:
+      - '**' # Run on any branch


To optimize CI usage and reduce resource consumption, consider triggering this workflow on pull_request events instead of every push to every branch ('**'). This ensures that linting is performed on changes that are ready for review and merging.

If the goal is to allow developers to manually trigger it for debugging, add a workflow_dispatch trigger.

on: pull_request: workflow_dispatch:

vr-varad · 2025-08-05T21:03:11Z

Thank you for your contribution!!
If this work is complete, this is a good item to add to the weekly Meshery Development meeting agenda. You can add this item in the doc, attend, and present it. Meeting details at https://meet.layer5.io/meshery.

ShigrafS · 2025-08-06T13:29:07Z

Hi @vr-varad
Actually the issue that this PR is trying to solve has a varied scope.
Therefore, I'm planning to reduce the scope of this PR and fix the original issue with multiple PRs.
Additionally, there are more changes that needs to be pushed to this branch, after I decide on my approach.

Please let me know your opinions.
Thanks!

lekaf974 · 2025-09-05T20:54:46Z

@ShigrafS I strongly suggest you create several PRs and fix issues 1 by 1 please, this will help reviewers

lekaf974

Could you adress comments

utils/utils.go

utils/unarchive.go

utils/walker/git.go

generators/github/git_repo.go

utils/unarchive.go

lekaf974

@ShigrafS just made some comments

utils/unarchive.go

Signed-off-by: ShigrafS <[email protected]>

Signed-off-by: SHIGRAF SALIK <[email protected]>

Signed-off-by: ShigrafS <[email protected]>

lekaf974 · 2025-09-16T01:31:16Z

utils/kubernetes/kompose/convert.go

+
+// formatComposeFile takes in a pointer to the compose file byte array and formats it
+// so that it is compatible with Kompose. It expects a validated Docker Compose file.
+func formatComposeFile(yamlManifest *DockerComposeFile) error {


Have we evaluate the impact on services calling this function since the signature has changed ?

formatComposeFile is only called once from within the Convert function in this same file.

you're right, I did not notice it was a local function. Sorry for the noise

lekaf974 · 2025-09-16T01:34:03Z

utils/walker/git.go

@@ -175,7 +182,7 @@ func clonewalk(g *Git) error {
 	}

 	if !info.IsDir() {
-		err = g.readFile(info, rootPath)
+		err = g.readFile(info, rootPath, log)


why adding logger, here ?

lekaf974 · 2025-09-16T01:35:04Z

@ShigrafS sorry for dealy and thanks for the work done, added small comments

lekaf974

changes to ensure files are closed properly

lekaf974 · 2025-09-16T01:40:16Z

utils/kubernetes/kompose/convert.go

-type composeFile struct {
-	Version string `yaml:"version,omitempty"`
+// cleanup removes temporary files
+func cleanup(tempFilePath, resultFilePath string) error {


I don't see the advantage to extract this in a dedicate function since it is bind to those files.

Also it should be done in a defer like previous implementation to ensure the files are closed properly. Here if one of the previous action fail the files will never be closed

No more used, write ?

Signed-off-by: ShigrafS <[email protected]>

lekaf974

Small comments added

lekaf974 · 2025-09-16T23:43:00Z

utils/kubernetes/kompose/convert.go

+			// A logger should be used here to warn about the failed cleanup.
+			// e.g., log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err)


Why not log here ?

lekaf974 · 2025-09-16T23:43:07Z

utils/kubernetes/kompose/convert.go

+			// e.g., log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err)
+		}
+		if err := os.Remove(resultFilePath); err != nil && !os.IsNotExist(err) {
+			// e.g., log.Warnf("failed to remove result file %s: %v", resultFilePath, err)


lekaf974 · 2025-09-16T23:43:21Z

utils/kubernetes/kompose/convert.go

@@ -42,37 +43,55 @@ func Convert(dockerCompose DockerComposeFile) (string, error) {
 	tempFilePath := filepath.Join(mesheryDir, "temp.data")
 	resultFilePath := filepath.Join(mesheryDir, "result.yaml")

+	// Defer cleanup to ensure temp files are removed even on error.


not util IMO

lekaf974 · 2025-09-16T23:44:10Z

utils/kubernetes/kompose/convert.go

-type composeFile struct {
-	Version string `yaml:"version,omitempty"`
+// cleanup removes temporary files
+func cleanup(tempFilePath, resultFilePath string) error {


No more used, write ?

lekaf974 · 2025-09-16T23:46:31Z

utils/kubernetes/kompose/convert.go

+
+// formatComposeFile takes in a pointer to the compose file byte array and formats it
+// so that it is compatible with Kompose. It expects a validated Docker Compose file.
+func formatComposeFile(yamlManifest *DockerComposeFile) error {


you're right, I did not notice it was a local function. Sorry for the noise

ShigrafS force-pushed the lint-fix branch from f5034a9 to 3378a04 Compare July 27, 2025 09:59

gemini-code-assist bot reviewed Jul 27, 2025

View reviewed changes

ShigrafS force-pushed the lint-fix branch 2 times, most recently from 209f155 to 77dbf28 Compare September 8, 2025 12:16

lekaf974 requested changes Sep 8, 2025

View reviewed changes

ShigrafS marked this pull request as ready for review September 8, 2025 14:25

ShigrafS requested a review from lekaf974 September 9, 2025 16:37

lekaf974 reviewed Sep 10, 2025

View reviewed changes

utils/unarchive.go Outdated Show resolved Hide resolved

lekaf974 reviewed Sep 10, 2025

View reviewed changes

utils/unarchive.go Outdated Show resolved Hide resolved

lekaf974 reviewed Sep 10, 2025

View reviewed changes

utils/unarchive.go Outdated Show resolved Hide resolved

lekaf974 requested changes Sep 10, 2025

View reviewed changes

lekaf974 reviewed Sep 10, 2025

View reviewed changes

utils/unarchive.go Outdated Show resolved Hide resolved

ShigrafS and others added 3 commits September 15, 2025 19:10

refactor(file-handling): improve error handling and file cleanup

1f71e83

Signed-off-by: ShigrafS <[email protected]>

fix: switched to meshkit loggers and addressed other review comments

eefc216

Signed-off-by: SHIGRAF SALIK <[email protected]>

fix: switched to meshkit logger and other fixes

67c1f36

Signed-off-by: SHIGRAF SALIK <[email protected]>

ShigrafS force-pushed the lint-fix branch from f395a00 to 67c1f36 Compare September 15, 2025 13:40

ShigrafS added 2 commits September 15, 2025 19:28

feat: added new error

a499769

Signed-off-by: ShigrafS <[email protected]>

fix: addressed review comments

41f3586

Signed-off-by: ShigrafS <[email protected]>

ShigrafS requested a review from lekaf974 September 15, 2025 15:10

lekaf974 reviewed Sep 16, 2025

View reviewed changes

lekaf974 requested changes Sep 16, 2025

View reviewed changes

refactor: revert to defer

ac17449

Signed-off-by: ShigrafS <[email protected]>

lekaf974 requested changes Sep 16, 2025

View reviewed changes

		// A logger should be used here to warn about the failed cleanup.
		// e.g., log.Warnf("failed to remove temporary file %s: %v", tempFilePath, err)

fix: resolve warnings and improve file extraction logic #773

Are you sure you want to change the base?

fix: resolve warnings and improve file extraction logic #773

Uh oh!

Conversation

ShigrafS commented Jul 27, 2025

Uh oh!

welcome bot commented Jul 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 27, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 27, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 27, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 27, 2025

Choose a reason for hiding this comment

Uh oh!

vr-varad commented Aug 5, 2025

Uh oh!

ShigrafS commented Aug 6, 2025

Uh oh!

lekaf974 commented Sep 5, 2025

Uh oh!

lekaf974 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lekaf974 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lekaf974 commented Sep 16, 2025

Uh oh!

lekaf974 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lekaf974 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!