Move version to shared config file, warn on mismatch

mrheinen commented 2 months ago

PR Type

Enhancement, Bug fix

Description

Implemented a centralized version management system using a shared constants file
Added version compatibility check between backend and honeypot
Refactored agent and backend code to use the new shared version constant
Created new utility functions for version comparison
Updated tests to reflect the new version handling
Added new dependencies for semantic versioning (github.com/blang/semver/v4)
Updated Bazel build files to include new files and dependencies
Improved error handling for version mismatches

Changes walkthrough 📝

Relevant files

Enhancement

4 files

agent.go `Update agent version handling` pkg/agent/agent.go Removed hardcoded `AgentVersion` constant Imported `constants` package Updated `SendStatus` to use `constants.LophiidVersion`	+2/-4
backend.go `Implement version compatibility check` pkg/backend/backend.go Imported `constants` package Added version compatibility check in `SendStatus` function Updated honeypot insertion to use `req.GetVersion()`	+9/-0
shared_constants.go `Add shared version constant` pkg/util/constants/shared_constants.go - Added new file with `LophiidVersion` constant	+5/-0
versioning.go `Implement version compatibility check` pkg/util/versioning.go Added new file with `IsLophiidVersionCompatible` function Implemented version compatibility check logic	+43/-0

Tests

2 files

backend_test.go `Update backend tests for version changes` pkg/backend/backend_test.go Updated test cases to use `constants.LophiidVersion` Imported `constants` package	+10/-8
versioning_test.go `Add tests for version compatibility` pkg/util/versioning_test.go Added new file with tests for `IsLophiidVersionCompatible` function	+67/-0

Dependencies

3 files

deps.bzl `Update Bazel dependencies` deps.bzl - Added dependency for `github.com/blang/semver/v4`	+7/-0
go.mod `Update Go module dependencies` go.mod - Added dependency for `github.com/blang/semver/v4`	+1/-0
go.sum `Update Go module checksums` go.sum - Added checksum for `github.com/blang/semver/v4`	+2/-0

Configuration changes

4 files

BUILD.bazel `Update Bazel build for agent` pkg/agent/BUILD.bazel - Added dependency on `//pkg/util/constants`	+1/-0
BUILD.bazel `Update Bazel build for backend` pkg/backend/BUILD.bazel - Added dependency on `//pkg/util/constants`	+1/-0
BUILD.bazel `Update Bazel build for util package` pkg/util/BUILD.bazel Added `versioning.go` and `versioning_test.go` to the build Added dependency on `@com_github_blang_semver_v4//:go_default_library`	+3/-0
BUILD.bazel `Add Bazel build for constants package` pkg/util/constants/BUILD.bazel - Added new Bazel build file for constants package	+8/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Error Handling
The error from `GetHoneypotByIP` is not properly handled. The code continues to insert a new honeypot even if an error occurs. Version Mismatch Handling
The version mismatch is only logged as a warning. Consider implementing a more robust handling mechanism or returning an error to the agent.

github-actions[bot] commented 2 months ago

Preparing PR description...

github-actions[bot] commented 2 months ago

Preparing review...

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	✅ Use standard semantic versioning format for the version constant ___ The version string "0.10a" does not follow semantic versioning conventions. Consider using a standard semantic version format like "0.10.0" or "0.10.0-alpha" for better compatibility and version management. [pkg/util/constants/shared_constants.go [5]](https://github.com/mrheinen/lophiid/pull/12/files#diff-09d5df4a356db915ba50a2adae84a5db4eca7cae2f51137e5a1b0c20494fd18aR5-R5) ```diff -const LophiidVersion = "0.10a" +const LophiidVersion = "0.10.0-alpha" ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 9 Why: Adopting a standard semantic versioning format is crucial for version management and compatibility across the project.	9
Enhancement	✅ Use semantic versioning for more flexible version comparison ___ Suggestion Impact: The commit replaced the direct string comparison of versions with a function call to check compatibility, aligning with the suggestion to use semantic versioning for more flexible version comparison. code diff: ```diff + if err := util.IsLophiidVersionCompatible(req.GetVersion(), constants.LophiidVersion); err != nil { + slog.Error("backend and honeypot version are incompatible", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion()), slog.String("error", err.Error())) ``` ___ Consider using a semantic version comparison instead of a direct string comparison. This would allow for more flexible version checking, such as allowing minor version differences. [pkg/backend/backend.go [307-309]](https://github.com/mrheinen/lophiid/pull/12/files#diff-c65bcfe9bb457434c3e69ba3f0576d7669935f350d24e2c2c58b05b4f9c510b2R307-R309) ```diff -if req.GetVersion() != constants.LophiidVersion { - slog.Warn("backend version differs from honeypot version", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion())) +if !semver.IsCompatible(req.GetVersion(), constants.LophiidVersion) { + slog.Warn("backend version incompatible with honeypot version", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion())) } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion proposes a more robust method for version comparison, which would improve flexibility and maintainability of the code.	8
Consistency	Add version mismatch check in the agent to match the backend's behavior ___ Consider adding a version check in the agent similar to the one in the backend. This would ensure that both the agent and backend are aware of any version mismatches. [pkg/agent/agent.go [98-101]](https://github.com/mrheinen/lophiid/pull/12/files#diff-18bf746206c8ac217eb16ffb0cf91a6e676e05a7b517dc70aef0260d0871028fR98-R101) ```diff resp, err := a.backendClient.SendStatus(&backend_service.StatusRequest{ Ip: a.reportIP, Version: constants.LophiidVersion, }) +if err == nil && resp.BackendVersion != constants.LophiidVersion { + slog.Warn("agent version differs from backend version", slog.String("agent_version", constants.LophiidVersion), slog.String("backend_version", resp.BackendVersion)) +} ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: This suggestion improves consistency between agent and backend, enhancing overall system coherence and error detection.	7
Code structure	✅ Reorder operations to perform error handling before version check ___ Suggestion Impact: The commit reordered the version compatibility check to occur before the error handling for the database operation, which aligns with the suggestion to handle errors before performing the version check. code diff: ```diff + + // Right now we just print an error because it's actually useful to still + // update the database with this honeypots information. + if err := util.IsLophiidVersionCompatible(req.GetVersion(), constants.LophiidVersion); err != nil { + slog.Error("backend and honeypot version are incompatible", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion()), slog.String("error", err.Error())) + } ``` ___ The version check is performed before the error check for `s.dbClient.GetHoneypotByIP(req.GetIp())`. Consider moving the version check after the error handling to ensure it's only performed when there's no error retrieving the honeypot data. [pkg/backend/backend.go [303-312]](https://github.com/mrheinen/lophiid/pull/12/files#diff-c65bcfe9bb457434c3e69ba3f0576d7669935f350d24e2c2c58b05b4f9c510b2R303-R312) ```diff dm, err := s.dbClient.GetHoneypotByIP(req.GetIp()) if err != nil { + _, err := s.dbClient.Insert(&database.Honeypot{ + IP: req.GetIp(), + Version: req.GetVersion(), + LastCheckin: time.Now(), + }) + if err != nil { + return nil, err + } +} - // For now we just warn if an agent is not the same version of the backend. - if req.GetVersion() != constants.LophiidVersion { - slog.Warn("backend version differs from honeypot version", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion())) - } - _, err := s.dbClient.Insert(&database.Honeypot{ +// For now we just warn if an agent is not the same version of the backend. +if req.GetVersion() != constants.LophiidVersion { + slog.Warn("backend version differs from honeypot version", slog.String("backend_version", constants.LophiidVersion), slog.String("honeypot_version", req.GetVersion())) +} ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion improves code structure and error handling, but the impact is relatively minor compared to other suggestions.	6

codiumai-pr-agent-pro[bot] commented 2 months ago

CI Failure Feedback 🧐

**Action:** build

**Failed stage:** [Build](https://github.com/mrheinen/lophiid/actions/runs/10405367478/job/28815939437) [❌]

**Failed test name:** GoTestGenTest pkg/util/util_test_/testmain.go

**Failure summary:** The action failed due to missing input files in the Go test generation process:

The file versioning.go was missing from the //pkg/util directory.

The file versioning_test.go was also missing from the //pkg/util directory.

These missing files caused the GoTestGenTest step to fail for pkg/util/util_test_/testmain.go.

The build process could not complete successfully due to these missing input files.

Relevant error logs:

```yaml 1: ##[group]Operating System 2: Ubuntu ... 557: Analyzing: 29 targets (127 packages loaded, 10412 targets configured) 558: [4 / 8] GoStdlib external/io_bazel_rules_go/stdlib_/pkg; 6s linux-sandbox 559: Analyzing: 29 targets (131 packages loaded, 10496 targets configured) 560: [4 / 8] GoStdlib external/io_bazel_rules_go/stdlib_/pkg; 7s linux-sandbox 561: Analyzing: 29 targets (161 packages loaded, 10661 targets configured) 562: [6 / 28] GoStdlib external/io_bazel_rules_go/stdlib_/pkg; 8s linux-sandbox 563: Analyzing: 29 targets (164 packages loaded, 10677 targets configured) 564: [6 / 28] GoStdlib external/io_bazel_rules_go/stdlib_/pkg; 9s linux-sandbox 565: ERROR: /home/runner/work/lophiid/lophiid/pkg/util/BUILD.bazel:18:8: GoTestGenTest pkg/util/util_test_/testmain.go failed: missing input file '//pkg/util:versioning.go' 566: ERROR: /home/runner/work/lophiid/lophiid/pkg/util/BUILD.bazel:18:8: GoTestGenTest pkg/util/util_test_/testmain.go failed: missing input file '//pkg/util:versioning_test.go' 567: ERROR: /home/runner/work/lophiid/lophiid/pkg/util/BUILD.bazel:18:8: GoTestGenTest pkg/util/util_test_/testmain.go failed: 2 input file(s) do not exist 568: Use --verbose_failures to see the command lines of failed build steps. 569: ERROR: /home/runner/work/lophiid/lophiid/pkg/util/BUILD.bazel:18:8 GoTestGenTest pkg/util/util_test_/testmain.go failed: 2 input file(s) do not exist 570: INFO: Elapsed time: 61.945s, Critical Path: 40.16s 571: INFO: 12 processes: 11 internal, 1 linux-sandbox. 572: ERROR: Build did NOT complete successfully 573: FAILED: 574: ##[error]Process completed with exit code 1. ```

✨ CI feedback usage guide:

The CI feedback tool (`/checks)` automatically triggers when a PR has a failed check. The tool analyzes the failed checks and provides several feedbacks: - Failed stage - Failed test name - Failure summary - Relevant error logs In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: ``` /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}" ``` where `{repo_name}` is the name of the repository, `{run_number}` is the run number of the failed check, and `{job_number}` is the job number of the failed check. #### Configuration options - `enable_auto_checks_feedback` - if set to true, the tool will automatically provide feedback when a check is failed. Default is true. - `excluded_checks_list` - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list. - `enable_help_text` - if set to true, the tool will provide a help message with the feedback. Default is true. - `persistent_comment` - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true. - `final_update_message` - if `persistent_comment` is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true. See more information about the `checks` tool in the [docs](https://pr-agent-docs.codium.ai/tools/ci_feedback/).

mrheinen commented 2 months ago

/describe

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Description updated to latest commit (https://github.com/mrheinen/lophiid/commit/b18b2d885e1f06975207945e3867166b13345cee)

[ ] Copy walkthrough table to "Files Changed" Tab

github-actions[bot] commented 2 months ago

Preparing PR description...

mrheinen commented 2 months ago

In this case the PR agent gave a good suggestion regarding the versioning. I didn't apply the suggested change because it was incomplete and it was better implemented manually.

mrheinen / lophiid