Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in asset's semver comparison #240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flatrow
Copy link

@flatrow flatrow commented May 20, 2020

As stated in api.md asset version id is formatted as name_flavor, resulting in TypeError: Invalid version: 0.0.2_default when more than one latestVersion asset is available.

@DustinBrett
Copy link
Collaborator

So this is for comparison within the same flavor? Not comparing different flavors? If so, then that seems like a valid bug.

@flatrow
Copy link
Author

flatrow commented May 20, 2020

As I see versions are filtered by flavor at line 352 of VersionController:

          .find(UtilityService.getTruthyObject({
            channel: applicableChannels,
            createdAt: createdAtFilter,
            availability: availabilityFilter(),
            flavor
          }))

Though I can double check with different flavors because I have used only default, but anyway semver cannot compare anything but versions and that's what fix is about.

@DustinBrett
Copy link
Collaborator

Ya I agree the semver fix makes sense. I just had a concern about mixing up flavors as they are not meant to be compared.

@flatrow
Copy link
Author

flatrow commented May 20, 2020

Yeah, I understand your concern and will test this scenario in a few days.

@stale
Copy link

stale bot commented Jul 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 19, 2020
@stale stale bot closed this Jul 27, 2020
@ArekSredzki ArekSredzki reopened this Mar 13, 2021
@ArekSredzki
Copy link
Owner

@flatrow Did you ever get a chance to test that edge case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants