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

allow to add dependencies and php extensions into the Nextcloud container #1377

Merged
merged 3 commits into from
Nov 10, 2022

Conversation

szaimen
Copy link
Collaborator

@szaimen szaimen commented Nov 8, 2022

Close #1162

Signed-off-by: Simon L [email protected]

@szaimen szaimen added the 2. developing Work in progress label Nov 8, 2022
@szaimen szaimen added this to the next milestone Nov 8, 2022
@szaimen szaimen force-pushed the enh/1162/allow-to-add-additional-dependencies branch from 4098d5d to 00a85a1 Compare November 8, 2022 20:49
@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request and removed 2. developing Work in progress labels Nov 8, 2022
@szaimen szaimen marked this pull request as ready for review November 8, 2022 20:58
@szaimen
Copy link
Collaborator Author

szaimen commented Nov 8, 2022

@Zoey2936 WDYT? :)

@szaimen szaimen requested a review from Zoey2936 November 8, 2022 20:58
Copy link
Collaborator

@Zoey2936 Zoey2936 left a comment

Choose a reason for hiding this comment

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

I think it should work, but I think it would also be good to have an option to install packages using

npm install --global <package> - requieres to have npm installed using apk
yarn global add <package> - requieres to have yarn installed using apk
pip install <packages> - requieres to have py3-pip installed using apk

Because for example the NCDownloader app requires installing yt-dlp using pip (which requires gcc, musl-dev and py3-pip to be installed using apk)
I currently don't know if any npm/yarn packages is needed by any nextcloud app existing, but I think at some point in the future it could be also needed

Containers/nextcloud/start.sh Outdated Show resolved Hide resolved
@szaimen
Copy link
Collaborator Author

szaimen commented Nov 8, 2022

Because for example the NCDownloader app requires installing yt-dlp using pip (which requires gcc, musl-dev and py3-pip to be installed using apk)

https://pkgs.alpinelinux.org/packages?name=youtube-dl&branch=v3.16&repo=&arch=&maintainer=

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Nov 8, 2022

Because for example the NCDownloader app requires installing yt-dlp using pip (which requires gcc, musl-dev and py3-pip to be installed using apk)

https://pkgs.alpinelinux.org/packages?name=youtube-dl&branch=v3.16&repo=&arch=&maintainer=

There is a difference between youtube-dl and yt-dlp
And NCDownloader referces to yt-dlp

@szaimen
Copy link
Collaborator Author

szaimen commented Nov 8, 2022

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Nov 8, 2022

Then apk packages and php extension should be enough, then there is only the --no-cache question, if it is needed

Containers/nextcloud/start.sh Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@szaimen szaimen force-pushed the enh/1162/allow-to-add-additional-dependencies branch from 00a85a1 to da2b967 Compare November 10, 2022 11:32
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@szaimen szaimen force-pushed the enh/1162/allow-to-add-additional-dependencies branch from 17add46 to e720e07 Compare November 10, 2022 17:04
Signed-off-by: Simon L <[email protected]>
@szaimen szaimen force-pushed the enh/1162/allow-to-add-additional-dependencies branch from e720e07 to 135ed21 Compare November 10, 2022 17:09
Signed-off-by: Simon L <[email protected]>
@szaimen szaimen force-pushed the enh/1162/allow-to-add-additional-dependencies branch from 135ed21 to fb7d5e5 Compare November 10, 2022 17:13
@szaimen szaimen requested a review from Zoey2936 November 10, 2022 17:17
Copy link
Collaborator

@Zoey2936 Zoey2936 left a comment

Choose a reason for hiding this comment

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

Looks good

@szaimen
Copy link
Collaborator Author

szaimen commented Nov 10, 2022

Thanks Zoey! :)

@szaimen szaimen merged commit a049470 into main Nov 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the enh/1162/allow-to-add-additional-dependencies branch November 10, 2022 17:22
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
Signed-off-by: Simon L <[email protected]>
szaimen added a commit that referenced this pull request Nov 10, 2022
@Zoey2936
Copy link
Collaborator

I think numbers should also be allowed
grafik

@szaimen
Copy link
Collaborator Author

szaimen commented Nov 10, 2022

good catch. Let me add this...

@szaimen
Copy link
Collaborator Author

szaimen commented Nov 10, 2022

to review: #1387

szaimen added a commit that referenced this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Think about a solution for adding external dependencies into the Nextcloud container
2 participants