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.

Sign up for GitHub

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

Jump to bottom

Do not update apps if it comes from git #7505

Merged
merged 2 commits into from
Dec 18, 2017
Merged

Do not update apps if it comes from git #7505

merged 2 commits into from
Dec 18, 2017

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Dec 14, 2017 •
edited by MorrisJobke
Loading

We had a lot of issues as devs by those auto update...

Fixes #7011

@skjnldsv
Do not update apps if it comes from git
cca8744 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added this to the Nextcloud 13 milestone Dec 14, 2017
@skjnldsv skjnldsv self-assigned this Dec 14, 2017
MorrisJobke
MorrisJobke reviewed Dec 14, 2017
View reviewed changes
lib/private/Installer.php
@@ -399,6 +399,10 @@ public function isUpdateAvailable($appId) {
$apps = $this->appFetcher->get();
}

if ($this->isInstalledFromGit($appId) === true) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this check one up ... just to return earlier in the method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course!

@MorrisJobke MorrisJobke mentioned this pull request Dec 14, 2017
Closed
@skjnldsv
Up check
2dbd505 
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
MorrisJobke
MorrisJobke approved these changes Dec 14, 2017
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke requested a review from blizzz December 14, 2017 09:08
rullzer
rullzer approved these changes Dec 14, 2017
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Works for the 99% use case for me! Lets do this.

@ChristophWurst
Copy link
Member

What happens if a developer accidentally packages the .git dir? Does that mean the app will never be updated again? 🙈

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 14, 2017
@skjnldsv
Copy link
Member Author

@ChristophWurst Yup!
We should add a test in the appstore for that I think! :)

@ChristophWurst ChristophWurst mentioned this pull request Dec 14, 2017
Open
@MorrisJobke MorrisJobke mentioned this pull request Dec 14, 2017
Closed
@MorrisJobke
Copy link
Member

We should add a test in the appstore for that I think! :)

nextcloud/appstore#537

@codecov
Copy link

codecov bot commented Dec 14, 2017 •
edited
Loading

Codecov Report

Merging #7505 into master will increase coverage by 0.05%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master    #7505      +/-   ##
============================================
+ Coverage     51.11%   51.16%   +0.05%     
+ Complexity    24903    24868      -35     
============================================
  Files          1601     1601              
  Lines         94778    94691      -87     
  Branches       1368     1368              
============================================
+ Hits          48445    48452       +7     
+ Misses        46333    46239      -94
Impacted Files Coverage Δ Complexity Δ
lib/private/Installer.php 58.17% <75%> (+0.52%) 77 <2> (+3) ⬆️
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)
lib/private/Server.php 80.9% <0%> (-0.12%) 134% <0%> (ø)
...rver_connector/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
...pps/encryption/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
...files_versions/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
apps/oauth2/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
.../files_sharing/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
...tenotification/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
apps/testing/composer/composer/autoload_real.php 0% <0%> (ø) 9% <0%> (-2%) ⬇️
... and 14 more

@skjnldsv
Copy link
Member Author

We could also refuse to install an app if it has a .git in it ;)

@MorrisJobke
Copy link
Member

What happens if a developer accidentally packages the .git dir? Does that mean the app will never be updated again? 🙈

@ChristophWurst @skjnldsv Another idea: only reject this if the nextcloud itself is also a git clone (update channel === git) 😉

@MorrisJobke
Copy link
Member

@BernhardPosselt Mentioned that we maybe should also add checks for other typical VCS systems and have a blacklist:

  • SVN
  • HG
  • bazaar
  • git

Anything else?

@skjnldsv
Copy link
Member Author

@ChristophWurst @skjnldsv Another idea: only reject this if the nextcloud itself is also a git clone (update channel === git) 😉

@MorrisJobke I would say no, because I sometimes uses default apps without using their git repo :)

@jancborchardt
Copy link
Member

cc @Henni for review as he also had this issue :)

@MorrisJobke
Copy link
Member

Okay - let's go for the .git folder for now and check if this is already enough.

@MorrisJobke MorrisJobke merged commit 97f80f5 into master Dec 18, 2017
@MorrisJobke MorrisJobke deleted the do-not-update-if-app-comes-from-git branch December 18, 2017 12:44
@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer rullzer approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@blizzz blizzz Awaiting requested review from blizzz

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish feature: apps management high technical debt
Projects
None yet
Milestone
Nextcloud 13
Development

Successfully merging this pull request may close these issues.
5 participants
@skjnldsv @ChristophWurst @MorrisJobke @jancborchardt @rullzer