feat(daemon): add --show-deploy-config flag to daemon:list command#791
feat(daemon): add --show-deploy-config flag to daemon:list command#791isaaclee12 wants to merge 1 commit intonextcloud:mainfrom
Conversation
Signed-off-by: Isaac Lee <isaac.wonha.lee@outlook.com>
cf615a6 to
9ce4f13
Compare
|
Hey Isaac, thanks for the PR and welcome to Nextcloud contributions! The idea behind this is solid and matches what was requested in the issue. I have a few things that need to be changed before we can merge this though. First, please revert all the code style changes. The brace placement in our project is same-line (opening brace on the same line as the class/method declaration), and your PR switches several of them to next-line style. Same thing with the constructor parameter alignment change. Just keep the existing style as-is and only touch what is needed for the feature. Second, the deploy config output is incomplete. You are only outputting net, Third, putting JSON_PRETTY_PRINT inside a table cell is going to look really bad in the terminal. The table already has 10+ columns and a multi-line JSON blob in one cell will break the layout. I would suggest one of two approaches: either use compact (single-line) JSON in the table column, or better yet, when the flag is passed, skip the table entirely and just output each daemon as a formatted JSON block. The second option would actually be more readable and useful for people who want to inspect the full config. Also a small PHP style thing: use And last, please make sure to test this locally before resubmitting. Setting up Thanks again for picking this up. The feature is definitely wanted, just needs some adjustments. Let me know if you have any questions. |
Relevant Issue: #785
This is my first PR to Nextcloud!
NOTES:
The force push below is due to the DCO check failing. I ran
git commit --amend -sandgit push --force-with-lease origin feat/785/add-deploy-config-infosince there's only one commit from me.I have yet to set up
nextcloud-docker-devon my machine, so I haven't been able run tests on this yet. I'm hoping I can see the results in GitHub Actions once those workflows are kicked off.