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

[For 10.4] Filter app:list for just enabled or disabled apps #36520

Merged
merged 3 commits into from
Dec 17, 2019

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
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
Contributor

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
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...

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

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
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.

4 participants