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

COPS: Update to version 1.5.4 #5957

Merged
merged 7 commits into from
Dec 15, 2023
Merged

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Dec 12, 2023

Description

This PR contains the update to the last published version of COPS which is compatible with PHP 7.4. The subsequent versions split off resources in preparation of 2.x release. The 2.x.x releases will support PHP >= 8.1 and requires a newer version of PHP.

This pull request introduces enhancements for DSM 6 installations, streamlining the user experience by automatically configuring necessary PHP settings. Additionally, it incorporates a data share worker to automate the configuration of permissions, further improving the overall quality of life for users.

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Package update

- Implement web service resource worker
- Implement DSM 6 web and php configs
@mreid-tt mreid-tt self-assigned this Dec 12, 2023
Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

please keep changes minimal

  • why did you change the order of the lines in app/config?
  • why did you add variables for jq, sed and chmod?
  • why did you replace cpby ${CP}? (this is not the same, CP = cp -rfp)
  • why did you merge the comments regarding config and .htaccess file together?
  • ...

this makes it difficult to review the real changes.

if you do this for the sake of adjusting different packages with web apps, then please do not do this on regular package updates, but create a dedicated PR with all affected packages - but without changing the SPK_REV of SPK_VERS nor publishing the packages.

But there is more to do for web app packages:
I hope we can create generic helper functions for packages with apache_php configuration.

@mreid-tt
Copy link
Contributor Author

please keep changes minimal

apologies, I didn't consider the challenge I would create by making a bunch of tweaks in addition to substantive changes. I will try to be more considerate in the future.

  • why did you change the order of the lines in app/config?

This was cosmetic to more closely match the order presented in your demoservice and demowebservice examples which I modelled most of the packages I've worked on recently.

  • why did you add variables for jq, sed and chmod?

This was also cosmetic similar to those introduced in the ownCloud modernisation. I liked the idea of using variables for executables so that I could easily identify when an execution is made with any special parameters.

  • why did you replace cpby ${CP}? (this is not the same, CP = cp -rfp)

Agreed that it is not the same but I found the added parameters in the framework useful to preserve attributes. This was also my preference for using variables for executables.

  • why did you merge the comments regarding config and .htaccess file together?

I didn't like original code which included separate variables for the paths and the filenames, so I collapsed these down to make more readable and similar to other service_save and service_restore functions I worked on recently.

  • ...

this makes it difficult to review the real changes.

I do appreciate that some of these cosmetic items are more about style and consistency with other packages so I'll try to be more considerate in the future.

if you do this for the sake of adjusting different packages with web apps, then please do not do this on regular package updates, but create a dedicated PR with all affected packages - but without changing the SPK_REV of SPK_VERS nor publishing the packages.

Ah! This makes sense. I was taking on modernising these older packages one at a time so I didn't take a holistic view of similar packages but will definitely consider this in the future.

But there is more to do for web app packages: I hope we can create generic helper functions for packages with apache_php configuration.

Is that related to the work I did for the DSM 6 workarounds to support the PHP configs?

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Dec 13, 2023

@hgy59, to streamline the service setup script, I've removed my cosmetic preference regarding variables for common executables.

@mreid-tt mreid-tt merged commit c5161a6 into SynoCommunity:master Dec 15, 2023
17 checks passed
@mreid-tt mreid-tt deleted the cops-update branch December 15, 2023 01:16
@mreid-tt mreid-tt added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/ready-to-merge status/published Published and activated (may take up to 48h until visible in DSM package manager) labels Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants