[BCF-3491] Add option to ignore pre-release version#21017
[BCF-3491] Add option to ignore pre-release version#21017cedric-cordenier merged 2 commits intodevelopfrom
Conversation
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to optionally ignore pre-release versions when comparing application and database versions. The change allows version checking to strip pre-release identifiers (e.g., "-cre" in "9.9.8-cre") when the CL_APP_VERSION_CHECK_IGNORE_PRE environment variable is set to "true".
Changes:
- Modified
CheckVersionfunction to accept anignorePrereleaseboolean parameter - Added logic to strip pre-release identifiers from both app and database versions when enabled
- Updated all test cases to pass the new
ignorePrereleaseparameter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/services/versioning/orm.go | Added ignorePrerelease parameter to CheckVersion function and implemented pre-release stripping logic; reads CL_APP_VERSION_CHECK_IGNORE_PRE environment variable |
| core/services/versioning/orm_test.go | Updated all CheckVersion test calls to include the new ignorePrerelease parameter; added test case for pre-release ignoring behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/services/versioning/orm.go
Outdated
| newDbv, err := dbv.SetPrerelease("") | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("could not generate db version without pre-release: %w", err) | ||
| } | ||
| dbv = &newDbv | ||
| newAppv, err := appv.SetPrerelease("") | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("could not generate app version without pre-release: %w", err) |
There was a problem hiding this comment.
Variable shadowing: the error variable 'err' is reused within the if block, which shadows the named return value 'err'. This could lead to confusion and potential bugs if the function's control flow changes. Consider using different variable names (e.g., 'dbErr' and 'appErr') for these intermediate error values.
| newDbv, err := dbv.SetPrerelease("") | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("could not generate db version without pre-release: %w", err) | |
| } | |
| dbv = &newDbv | |
| newAppv, err := appv.SetPrerelease("") | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("could not generate app version without pre-release: %w", err) | |
| newDbv, dbPrereleaseErr := dbv.SetPrerelease("") | |
| if dbPrereleaseErr != nil { | |
| return nil, nil, fmt.Errorf("could not generate db version without pre-release: %w", dbPrereleaseErr) | |
| } | |
| dbv = &newDbv | |
| newAppv, appPrereleaseErr := appv.SetPrerelease("") | |
| if appPrereleaseErr != nil { | |
| return nil, nil, fmt.Errorf("could not generate app version without pre-release: %w", appPrereleaseErr) |
| assert.Equal(t, "9.9.8", dbv.String()) | ||
|
|
||
| // ignores pre-release if specified | ||
| appv, dbv, err = CheckVersion(ctx, db, lggr, "9.9.8-cre", true) |
There was a problem hiding this comment.
The test only validates the happy path for pre-release ignoring. Consider adding test cases for error scenarios, such as when SetPrerelease fails or when comparing versions with different pre-release identifiers to ensure the feature works correctly in edge cases.
|
I see you updated files related to
|
![trunk-io[bot]](https://avatars.githubusercontent.com/in/120106?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cedric-cordenier
force-pushed
the
BCF-3491-ignore-pre-release-version
branch
from
9d6a353 to
61ec875
Compare
jmank88
left a comment
jmank88
left a comment
There was a problem hiding this comment.
How important is this in practice? Because it will inevitably cause problems too.
cedric-cordenier
force-pushed
the
BCF-3491-ignore-pre-release-version
branch
from
61ec875 to
07958f4
Compare
|
@jmank88 I think this is valuable -- we've been bitten multiple times by images that have inconsistently applied prerelease info in the semver tag. This is because in practice we use the prerelease tag to indicate the flavour of image, not that it's a pre-release. |
Semver has a spot for metadata, a suffix following a Regardless, should this be optional instead of affecting everyone? e.g. if it were opt-in via env var, we could set it in the released images that we build. |
|
@jmank88 It wouldn't because the problem is that we're sometimes setting it by accident. I can make it optional if you prefer, but fwiw I checked with @mateusz-sekara from CCIP and he said this wouldn't affect them negatively if we applied it across the board. |
The risk being introduced is to the normal usage though. This check is meaningful when semver is used normally |
| } | ||
|
|
||
| if dbvWithoutPre.GreaterThan(&appvWithoutPre) { | ||
| return nil, nil, errors.Errorf("Application version (%s) is lower than database version (%s). Only Chainlink %s or higher can be run on this database", appv, dbv, dbv) |
There was a problem hiding this comment.
the params to the error should be appvWithoutPre, dbvWithoutPre, dbvWithoutPre
cedric-cordenier
force-pushed
the
BCF-3491-ignore-pre-release-version
branch
from
07958f4 to
37f935f
Compare
|
5 participants
Address the PMAI to ignore the pre-release version