Skip to content
This repository has been archived by the owner on May 15, 2024. It is now read-only.

refactor(one-app-runner): use semver for app version comparison #608

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

10xLaCroixDrinker
Copy link
Member

Description

Refactor app version comparison to use semver

Motivation

Semver is more reliable than manual comparison

Test Conditions

Refactor - existing unit tests cover the change and still pass & added new test to cover prerelease use case

Types of changes

Check boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist

Check boxes that apply:

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

@10xLaCroixDrinker 10xLaCroixDrinker force-pushed the refactor/app-version-comparison branch from 990d97e to 94da5fa Compare February 9, 2024 00:27
@smackfu
Copy link
Member

smackfu commented Feb 9, 2024

There was some concern from @Matthew-Mallimo here that semver wouldn't work right if the image was just "one-app-dev:5"

#597 (comment)

@10xLaCroixDrinker
Copy link
Member Author

10xLaCroixDrinker commented Feb 9, 2024

There was some concern from @Matthew-Mallimo here that semver wouldn't work right if the image was just "one-app-dev:5"

#597 (comment)

There is unit test coverage for that use case. semver.intersects works fine for it

And as far as adding another dep, its already a dep of the bundler and I dont there are many cases of people installing the runner without that

@10xLaCroixDrinker
Copy link
Member Author

I will also be adding another case of version detection required for tracing support

Copy link
Member

@Matthew-Mallimo Matthew-Mallimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case where the supplied docker image is just one-app:5?

@10xLaCroixDrinker
Copy link
Member Author

@Matthew-Mallimo That test case already exists on line 411

@10xLaCroixDrinker 10xLaCroixDrinker merged commit ba59a59 into main Feb 21, 2024
5 checks passed
@10xLaCroixDrinker 10xLaCroixDrinker deleted the refactor/app-version-comparison branch February 21, 2024 22:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants