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

Loop for newest version in appstore response #2307

Merged
merged 2 commits into from
Nov 24, 2016
Merged

Conversation

LukasReschke
Copy link
Member

@LukasReschke LukasReschke commented Nov 24, 2016

The current implementation when fetching apps from the appstore is to assume that the first element is the newest version, this is now always applicable (e.g. daily builds etc.) and leads to the fact that for some apps (e.g. nextant) the newest version is not delivered. This can be easily tested by comparing the version of the downloaded Nextant version, without this patch it only delivers 0.6.6 instead of 0.10.0. (Category: Files)

This change will loop over all releases delivered by the appstore and chooses the newest compatible one. While not the cleanest solution, it does its job.

Most of the code are actually unit tests. Whereas I have copied the whole original response from the appstore and also have performed the transformation. So that's why the diff looks so huge.

Signed-off-by: Lukas Reschke lukas@statuscode.ch

The current implementation when fetching apps from the appstore is to assume that the first element is the newest version, this is now always applicable and leads to the fact that for some apps (e.g. nextant) the newest version is not delivered. This can be easily tested by comparing the version of the downloaded Nextant version.

This change will loop over all releases delivered by the appstore and chooses the newest compatible one. While not the cleanest solution, it does its job.

Most of the code are actually unit tests. Whereas I have copied the whole original response from the appstore and also have performed the transformation. So that's why the diff looks so huge.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke LukasReschke added 3. to review Waiting for reviews bug labels Nov 24, 2016
@LukasReschke LukasReschke added this to the Nextcloud 11.0 milestone Nov 24, 2016
@nickvergessen
Copy link
Member

Works, afterwards i am suggested an update to 0.10.0 on the category page, normal enabled page however does not.

The enabled page doesn't pass through "getAppsForCategory" thus it also needs to have that special logic applied.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@LukasReschke
Copy link
Member Author

normal enabled page however does not.

Good point. Adjusted the logic to also cover that. This controller file is really a little bit … special …

@nickvergessen
Copy link
Member

Works now 👍

@codecov-io
Copy link

codecov-io commented Nov 24, 2016

Current coverage is 57.04% (diff: 100%)

Merging #2307 into master will increase coverage by 0.01%

@@             master      #2307   diff @@
==========================================
  Files          1190       1190          
  Lines         71672      71717    +45   
  Methods        7292       7296     +4   
  Messages          0          0          
  Branches       1213       1213          
==========================================
+ Hits          40873      40912    +39   
- Misses        30799      30805     +6   
  Partials          0          0          

Powered by Codecov. Last update e3489d9...7162166

@blizzz
Copy link
Member

blizzz commented Nov 24, 2016

Code looks OK, works 👍

@blizzz blizzz merged commit cb69acc into master Nov 24, 2016
@blizzz blizzz deleted the better-filter-on-appstore branch November 24, 2016 15:57
@BernhardPosselt
Copy link
Member

Hm, this does not filter pre-releases, is this what you want? Users will be prompted to upgrade to 2.0.0-beta.1 for instance

@LukasReschke
Copy link
Member Author

Hm, this does not filter pre-releases, is this what you want? Users will be prompted to upgrade to 2.0.0-beta.1 for instance

https://github.com/nextcloud/server/pull/2307/files#diff-83840d74b03720feef2006e83453333eR83 should remove all nightlies. Am I missing something?

// Exclude all nightly releases
if($release['isNightly'] === false) {
// Exclude all versions not compatible with the current version
$versionParser = new VersionParser();
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed. All apps fetched from the API are working on the version provided in your URL

@BernhardPosselt
Copy link
Member

BernhardPosselt commented Nov 25, 2016

@LukasReschke right, however there are 3 "channels":

  • stable (e.g. 1.0.0)
  • pre-release (e.g. 1.0.0-beta)
  • nightly (e.g. 1.0.0-beta or 1.0.0 + nightly flag)

@LukasReschke
Copy link
Member Author

So for what does one have to filter? The docs doesn't tell anything about pre-releases etc…

(also it seems rather unnecessary for me at the moment since Nextcloud only supports the stable channel at the moment, but fair enough…)

@BernhardPosselt
Copy link
Member

Pre-releases in semver are denoted by having a - after the version string. Since we don't allow build metadata in our version you can simply check with

if (strpos($release['version'], '-') === false) {
    // not a pre-release
}

@BernhardPosselt
Copy link
Member

BernhardPosselt commented Nov 25, 2016

Versioning is documented at http://nextcloudappstore.readthedocs.io/en/latest/restapi.html#get-all-apps-and-releases

version
A semantic version without build metadata (e.g. 1.3.0, 1.2.1-alpha.1)

If you think this was not enough docs to handle this case, can you create a pull request to the docs that specifically adds a short description about pre-releases :D?

LukasReschke added a commit that referenced this pull request Nov 25, 2016
As SemVer can be used apps could define a release like "10.0.0-alpha". This is something that we don't support at the moment in the server and we should filter all prereleases.

Ref #2307 (comment)

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>
@nickvergessen
Copy link
Member

@BernhardPosselt Ever heard of -PL releases? just saying

@LukasReschke
Copy link
Member Author

#2327 should make you all happier… I hope…

@BernhardPosselt
Copy link
Member

@nickvergessen nope, havent heard of that one

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

Successfully merging this pull request may close these issues.

None yet

5 participants