fix(app): fixes lint issues

[mohit-nagaraj]

Description

This PR fixes #710

Notes for Reviewers

• io.Copy(buf, tr) needed to be checked for any potential errors that may arise
• We shuld use logrus.Infof for formatting %v err
• data, err := os.ReadFile(downloadfilePath) _, err = w.Write(data) the err is being over written here before it was checked so we shud break it into 2 separate checks

there are few more issues caused due to depreciation of the packages:
edit
updated the opa to use 1.x version
removed the deprecated packages in nat.go

Signed commits

• Yes, I signed my commits.

[gemini-code-assist]

Hello @mohit-nagaraj, 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!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly. This PR, titled "fix(app): fixes lint issues", aims to address several linting issues identified in the codebase. According to the description, it partially fixes issue #710. The changes primarily focus on improving error handling in specific areas and correcting logging format usage, as well as updating a dependency.

Highlights

• Improved Error Handling: Error checks have been added or corrected in converter/tests/helm_test.go for io.Copy and in generators/github/url.go for os.ReadFile before subsequent operations.
• Logging Format Correction: The logging call in utils/component/generator.go has been updated from logrus.Info to logrus.Infof to correctly format the log message with the provided error.
• Dependency Update: The go.sum file has been updated to include entries for github.com/meshery/schemas v0.8.22, likely as a result of other changes or dependency resolution during linting fixes.

Changelog

• converter/tests/helm_test.go
  • Added error check for io.Copy call on line 142.
• generators/github/url.go
  • Added an error check for os.ReadFile on line 58 before attempting to write the data.
• go.sum
  • Added entries for github.com/meshery/schemas v0.8.22.
• utils/component/generator.go
  • Changed logrus.Info to logrus.Infof on line 75 for proper error formatting.

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 is currently in preview and 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 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.

Footnotes

1. 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 addresses several linting issues as described, specifically focusing on error handling and a minor logging format change. The changes implemented are correct and improve the robustness and clarity of the code in the affected areas. It's good to see these specific issues being tackled.

Summary of Findings

• Error Handling (io.Copy): Added error checking for io.Copy in the extractManifestFromChart test helper function (medium severity).
• Error Handling (Overwritten Error): Corrected an issue in GetContent where the error from os.ReadFile was being overwritten by the error from w.Write before being checked. This fix ensures proper error propagation (high severity).
• Logging Format: Updated logrus.Info to logrus.Infof in IncludeComponentBasedOnGroup for correct formatted logging of errors (low severity - not commented on directly as per review settings).
• Deprecated Package Usages: The PR description and the provided golangci-lint output indicate several usages of deprecated packages (oras, nats, opa) that are not addressed in this PR. While outside the scope of the current changes, these should be addressed in follow-up PRs to fully resolve the reported lint issues.

Merge Readiness

The code changes in this pull request correctly address the specific error handling and logging issues they target. The fixes are well-implemented and improve the code quality in these areas. The PR description acknowledges that this is a partial fix for the overall linting problem, listing the remaining deprecated package issues. Based on the changes included in this PR, the code is in good shape to be merged. However, it is strongly recommended that follow-up pull requests are created to address the remaining deprecated package usages identified by the linter to fully resolve the linting debt. I am unable to approve the pull request; other reviewers should review and approve this code before merging.

[mohit-nagaraj]

@vishalvivekm could you suggest the next steps? shall i migrate few of the packges?

[mohit-nagaraj]

edited:
now its just cause of this 1 file:
broker/nats/nats.go lines 28, 60, 82, 87, 96, 108, 122
issue - nats.EncodedConn and related methods (NewEncodedConn, Close, Publish, etc.) are deprecated.

verifying with tests
they are failing for a whole lot of other reasons

[mohit-nagaraj]

updating the nats based on this doc
https://pkg.go.dev/github.com/nats-io/nats.go#section-readme

[mohit-nagaraj]

so the lints are in place, but the workflow will still fail because of the test cases which are failing

[mohit-nagaraj]

just checked out why one of the testcase is failing,
https://charts.bitnami.com/bitnami/consul
appears this chart has been moved to some other url or something

now we get only oci://registry-1.docker.io/bitnamicharts/consul:11.4.18 from the urls, so theres no direct https url to chart
https://repo.broadcom.com/bitnami-files/index.yaml

[mohit-nagaraj]

Bitnami (and many other Helm chart publishers) are moving from HTTP(S)-based chart repositories (e.g., https://charts.bitnami.com/bitnami/consul) to OCI-based registries (e.g., oci://registry-1.docker.io/bitnamicharts/consul:11.4.19). This is now the only way to get the latest charts, as confirmed in the Bitnami migration blog.

[mohit-nagaraj]

package_test.go:37: error while generating components: Get "oci://registry-1.docker.io/bitnamicharts/consul:11.4.19": unsupported protocol scheme "oci"

[mohit-nagaraj]

last test case which is failing
sanitization_test.go:186: Failed To Identify File as meshery-design , got , errors The file 'valid-design-oci.tar' was not recognized as any of the supported file types [meshery-design k8s-manifest helm-chart docker-compose k8s-kustomize]..Identification attempts and errors:.- Attempted to identify as 'docker-compose': failed to load Docker Compose config: yaml: control characters are not allowed.- Attempted to identify as 'k8s-kustomize': kustomization.yaml not found in extracted directory.- Attempted to identify as 'meshery-design': stat /home/codespace/.meshery/content: no such file or directory.- Attempted to identify as 'k8s-manifest': invalid file extension: .tar, only .yml and .yaml are supported.- Attempted to identify as 'helm-chart': failed to load Helm chart gzip: invalid header

for some reason its not able to locate these files i believe? should investigate this further

[mohit-nagaraj]

I am not able to find any in this route path for file 'schemas/constructs/v1beta1/designs.json' anyone knows where it has been moved to?

[mohit-nagaraj]

the original design file wasnt present
however found this file to resemble our PatternFile struct
https://github.com/meshery/schemas/blob/master/schemas/constructs/v1alpha2/design.json
so have taken the from from that place

[leecalcote]

Thanks for working on this area, @mohit-nagaraj.

Will you offer an explanation as to why this change on these lines is needed?

[mohit-nagaraj]

Hey so here, we have this map iterating over valid file extensions. this map is storing only valid file types, so theres no meaning in making it a map of string:bool. since all of them will be true anyways. coming to the actuallogic, SanitizeFile returns a error message not error code. Either we could define it there or the easiest approach was this.

In L48, i changed from error code to error message. This type is only used in one place that just for this one test case. hence made this change. in the message its like "expectedErrMsg: "The file 'valid.txt' could not be processed because the extension '.txt' is not supported by the system..The system is designed to handle files with the following extensions: .yaml, .tar, .tar.gz, .tgz, .zip, .json, .yml."," so when a slice is made the order in which it is generated is maintained when generating error

[mohit-nagaraj]

Just a quick update thought to put it in here
Refactor: Use Interface for NATS Connection

Previously, the Nats struct directly depended on the concrete *nats.Conn type from the NATS library. This tight coupling made it difficult to write effective unit tests, as it was not possible to substitute the real connection with a mock or fake. To address this, I refactored the code to introduce a NatsConn interface, which abstracts only the methods used by our implementation. The Nats struct now depends on this interface, allowing for greater flexibility, easier testing, and improved maintainability.

Test Coverage Improvements

With the introduction of the NatsConn interface, I was able to implement comprehensive unit tests for the NATS broker logic. By providing a mock implementation of the interface, the tests can simulate various scenarios and error conditions without requiring a real NATS server. This ensures that our code is robust and behaves as expected under different circumstances.

Test Cases Added and Coverage

The following test cases were added to ensure high coverage and reliability:

1. ConnectedEndpoints:
   Verifies that the correct list of endpoints is returned from the connection.

2. Info:
   Checks the connection information, including the case when the connection is not established.

3. CloseConnection:
   Ensures the connection is properly drained and closed.

4. Publish:

   • Tests successful message publishing.
   • Tests error handling for nil messages.
   • Tests error propagation from the underlying connection.

5. PublishWithChannel:
   Validates that messages sent through a channel are published correctly.

6. Subscribe:

   • Confirms that subscription logic works and messages are received.
   • Tests error handling when subscription fails.

7. SubscribeWithChannel:

   • Ensures messages are correctly unmarshaled and sent to the provided channel.
   • Tests error handling for subscription failures.

8. DeepCopy & DeepCopyObject:
   Verifies that deep copy methods create new, independent instances.

9. IsEmpty:
   Checks the logic for determining if the Nats struct is empty or uninitialized.

Lmk if i should add/modify something in here

[mohit-nagaraj]

{
Path: "catalog_data",
Resource: v1alpha1.CatalogData{
PublishedVersion: "v.10.9",
ContentClass: "sdsds",
Compatibility: []v1alpha1.CatalogDataCompatibility{
"kubernetes",
},
PatternCaveats: "NA",
PatternInfo: "NA",
Type: v1alpha1.CatalogDataType("Dployment"),
},
ShouldPass: false,
},

Still getting a error at this case tho, cause i could find something named catalog in v1beta1. (it had in v1beta2 not sure if we can use it cuz it was commited long ago). Have updated the other test case acc to v1beta1 design

[leecalcote]

Re-running workflows...

[Nitish-bot]

You can revert these changes as well, I think the entire fix for the sanitization test is a one-liner, but I could be wrong. Feel free to elaborate on your choice here.

[mohit-nagaraj]

Not sure how a map of all having bool values to be true is good over a slice which just tells which are the valid extensions. I dont get why we need to include and complicate stuff?

Mentioned it here as well: #738 (comment)

[Nitish-bot]

I would wait for Lee to clarify but from my understanding it's 2 things:

1. Don't fix what ain't broke
2. Out of scope for this PR

bonus: Fast lookups, not that it would matter in our case since we've got less than 10 entries