fix: compare infra versions semantically#5520
Open
immanuwell wants to merge 4 commits into
Open
Conversation
Signed-off-by: immanuwell <pchpr.00@list.ru>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes infra “latestVersion” selection in GetVersionDetails by replacing the previous numeric-flattening approach with a semantic (major/minor/patch) comparison, preventing cases like 3.29.0 incorrectly sorting above 4.0.0.
Changes:
- Replaced integer-flattening version logic with semantic parsing/comparison (
major.minor.patch). - Removed the old
fetchLatestVersion/updateVersionFormatlogic in favor ofcompareInfraVersions+parseInfraVersion. - Added unit tests asserting correct latest version selection for semantic versions and the
"ci"tag.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| chaoscenter/graphql/server/pkg/chaos_infrastructure/service.go | Implements semantic parsing/comparison for compatible infra versions and uses it to compute LatestVersion. |
| chaoscenter/graphql/server/pkg/chaos_infrastructure/service_test.go | Adds regression tests for semantic latest-version selection and "ci" behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Hey @immanuwell |
Signed-off-by: immanuwell <pchpr.00@list.ru>
immanuwell
force-pushed
the
fix/infra-version-comparison
branch
from
b86b3d7 to
df8f027
Compare
Author
|
@PriteshKiri done, I addressed all 3 suggestions |
| ContainerRuntimeExecutor string `required:"true" split_words:"true"` | ||
| WorkflowHelperImageVersion string `required:"true" split_words:"true"` | ||
| ChaosCenterUiEndpoint string `split_words:"true" default:"https://localhost:8080"` | ||
| ChaosGraphQLEndpoint string `split_words:"true" default:"http://chaos-litmus-server-service:9002"` |
Comment on lines
22
to
29
| func GetEndpoint(host string) (string, error) { | ||
| const apiPath = "/api/query" | ||
| const apiPath = "/query" | ||
| // returns endpoint from env, if provided by user | ||
| if utils.Config.ChaosCenterUiEndpoint != "" { | ||
| return strings.TrimRight(utils.Config.ChaosCenterUiEndpoint, "/") + apiPath, nil | ||
| if utils.Config.ChaosGraphQLEndpoint != "" { | ||
| return strings.TrimRight(utils.Config.ChaosGraphQLEndpoint, "/") + apiPath, nil | ||
| } | ||
|
|
||
| return strings.TrimRight(host, "/") + apiPath, nil |
Comment on lines
+14
to
16
| "getProject": {string(entities.RoleOwner), string(entities.RoleViewer), string(entities.RoleExecutor)}, | ||
| "getActiveProjectMembers": {string(entities.RoleOwner)}, | ||
| } |
Comment on lines
898
to
906
| // GetVersionDetails returns the compatible infra versions and the latest infra version supported for the current control plane version | ||
| func (in *infraService) GetVersionDetails() (*model.InfraVersionDetails, error) { | ||
| // Fetching the list of infra compatible versions | ||
| compatibleVersions := utils.Config.InfraCompatibleVersions | ||
|
|
||
| var compatibleArray []string | ||
| if err := json.Unmarshal([]byte(compatibleVersions), &compatibleArray); err != nil { | ||
| return &model.InfraVersionDetails{}, fmt.Errorf("failed to parse InfraCompatibleVersions config: %w", err) | ||
| } |
Contributor
|
Hey @immanuwell |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Fix latest infra version picking in
getVersionDetails. The old path flattenedx.y.zinto an int, so3.29.0became590and beat4.0.0as400. tiny edge case but real.Related: #4665, #4666
Repro:
INFRA_COMPATIBLE_VERSIONS='["3.29.0","4.0.0"]'GetVersionDetails/ GraphQLgetVersionDetails.latestVersionis3.29.0; after:4.0.0.Types of changes
Checklist
Dependency
None.
Special notes for your reviewer:
Tested with
go test ./pkg/chaos_infrastructure/... -count=1