Skip to content

Conversation

@phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 4, 2019

Description

Add options to the occ app:list command to display just the enabled or disabled apps.

This is on top of #36165

Motivation and Context

It is often convenient to be able to just display the enabled or disabled apps.

How Has This Been Tested?

Running acceptance tests locally.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

@phil-davis phil-davis requested a review from pako81 December 4, 2019 08:59
@phil-davis phil-davis self-assigned this Dec 4, 2019
@phil-davis
Copy link
Contributor Author

phil-davis commented Dec 4, 2019

ToDo if this seems like "a good thing":

  • add changelog item
  • raise doc issue
  • add new command options to docs

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #36520 into master will increase coverage by 0.01%.
The diff coverage is 94.73%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36520      +/-   ##
============================================
+ Coverage     64.66%   64.67%   +0.01%     
- Complexity    19049    19057       +8     
============================================
  Files          1269     1268       -1     
  Lines         74498    74504       +6     
  Branches       1311     1311              
============================================
+ Hits          48171    48184      +13     
+ Misses        25941    25934       -7     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.84% <94.73%> (+0.01%) 19057 <0> (+8) ⬆️
Impacted Files Coverage Δ Complexity Δ
core/Command/App/ListApps.php 92.3% <94.73%> (+1.92%) 25 <0> (+7) ⬆️
apps/files_sharing/ajax/publicpreview.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
config/config.apps.sample.php

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf40428...b989ae8. Read the comment docs.

@pako81
Copy link

pako81 commented Dec 4, 2019

@phil-davis was it agreed to show the version numbers when no --enabled or --disabled option is given in input?

As far as I understood from #36165 (comment), the expectation was to not show any version number for disabled apps when simply running app:list (the current behavior) but do so when --enabled or --disabled options are being used.

@phil-davis
Copy link
Contributor Author

was it agreed to show the version numbers when no --enabled or --disabled option is given in input?

I don't know what has been "agreed". With the current code occ app:list will now report version numbers for (some) disabled apps. That is a technical backward-compatibility-break for somebody who is parsing the text output. So it depends if we are generally promising exact backward-compatibility of the text output of occ commands.

We could easily change the logic so that disabled app versions are only reported when the user specifically does occ app:list --disabled

@phil-davis
Copy link
Contributor Author

I changed the logic so that the version of a disabled app is only displayed when --disabled is specifically requested.

occ app:list show no versions of disabled apps.
occ app:list --disabled shows versions of disabled apps.
occ app:list --disabled --enabled shows versions of disabled apps.

So you can get a full list of apps with versions by doing occ app:list --disabled --enabled

@phil-davis phil-davis requested a review from micbar December 4, 2019 12:10
@phil-davis
Copy link
Contributor Author

@micbar there has been various discussion about this in PR #36165
IMO the code here now does the job(s). It could easily go in 10.4.0 (I wrote acceptance tests as I adjusted the code) If you are happy with that, then add this to the 10.4.0 project.
And get whoever you think is appropriate to review.

@phil-davis
Copy link
Contributor Author

@micbar doco is reviewed and ready to merge.

Go ahead with this?

@micbar
Copy link
Contributor

micbar commented Dec 9, 2019

@phil-davis Yes, go ahead

@phil-davis
Copy link
Contributor Author

@micbar needs someone to review the code...

@micbar micbar requested review from VicDeo and sharidas December 9, 2019 17:47
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

See comments, rest LGTM

@phil-davis phil-davis requested a review from settermjd December 10, 2019 05:33
Copy link
Contributor

@settermjd settermjd left a comment

Choose a reason for hiding this comment

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

Just a few, minor, changes. I'll preemptively update https://github.com/owncloud/docs/pull/2144/files in line with my comments here.

@phil-davis phil-davis force-pushed the filter-enabled-disabled-apps branch from e1d3d63 to cc48f1f Compare December 12, 2019 09:01
@phil-davis phil-davis dismissed settermjd’s stale review December 12, 2019 09:02

changes have been done

@phil-davis
Copy link
Contributor Author

@micbar this is ready for you or some developer person to review the code.

@phil-davis phil-davis changed the title Filter app:list for just enabled or disabled apps [For 10.4] Filter app:list for just enabled or disabled apps Dec 12, 2019
Copy link
Contributor

@settermjd settermjd left a comment

Choose a reason for hiding this comment

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

What about noting that version numbers are displayed, along with when they are displayed for disabled apps which were previously enabled?

@phil-davis
Copy link
Contributor Author

Should we just do that in the docs?

settermjd added a commit to owncloud/docs that referenced this pull request Dec 12, 2019
After reviewing owncloud/core#36520, I felt that
the wording in this PR could be simpler still, hence this change.
@settermjd
Copy link
Contributor

settermjd commented Dec 13, 2019

Should we just do that in the docs?

Depending on how you look at it, both of these instances are docs. Some are "formal" docs, and some are informal. If the information's in one place, then it may be confusing if it's not in the other. Perhaps it's just me, but this is what I've found in a number of projects over the years, including ownCloud.

@phil-davis phil-davis merged commit 21ab2ff into master Dec 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the filter-enabled-disabled-apps branch December 17, 2019 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants