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

Add new nextcloud chart #5180

Closed
wants to merge 15 commits into from

Conversation

@affix
Copy link
Contributor

affix commented Apr 21, 2018

What this PR does / why we need it:
This PR adds a nextcloud chart to incubator

Nextcloud offers industry-leading on-premises file sync and online collaboration technology. Our expertise is in combining the convenience and ease of use of consumer-grade solutions like Dropbox and Google Drive with the security, privacy and control business needs.

Special notes for your reviewer:
This is loosely based around the owncloud chart, However I as many other prefer to use nextcloud for some of its optimisations.

Uses the official Nextcloud Docker image

@affix affix force-pushed the affix:nextcloud branch to f2ac2da Apr 21, 2018

@affix

This comment has been minimized.

Copy link
Contributor Author

affix commented Apr 21, 2018

/assign @mattfarina

@bbriggs

This comment has been minimized.

Copy link
Contributor

bbriggs commented Apr 23, 2018

I was literally writing a chart for that this weekend. Glad to see someone else took an interest too.

@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented Apr 23, 2018

/assign

@mattfarina
Copy link
Contributor

mattfarina left a comment

Thanks for submitting the pull request. I know of several people who want to use nextcloud on Kubernetes. This is great.

I did a quick review to provide some feedback.

sources:
- https://github.com/nextcloud/server
maintainers:
- name: Keiran Smith

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

The name should be your github username. That helps us to know who to contact when there's an issue with the chart.

- tompizmor
- sameersbn
- carrodher
- affix

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

For the time you can drop this file. It looks like you copied this from a chart managed by Bitnami. Since you're not a member of the Kubernetes org at the moment this isn't going to enable you to review or merge for the chart. Once you have some more contributions you can apply for membership and then a file like this will make sense.

resources:
requests:
memory: 512Mi
cpu: 300m

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

It is generally not advisable to provide hard resource limits to workloads and leave it configurable unless the workload requires such quantity bare minimum to function.

This is from the reviewers guide.

Can you comment these out unless this is the minimum to function.

##
ingress:
enabled: false
servicePort: 80

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

Does the container image allow for both http and https? It might be nice to allow for secure out of the box. That doesn't need to be part of the first PR but would be nice to add.

image:
registry: docker.io
repository: nextcloud
tag: latest

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

Can you update the image information to follow our standard layout? See the guide for more information and an example.

Also, please use a tagged version rather than latest. This will enable consistency between environments.

@@ -0,0 +1,28 @@
{{- if .Values.ingress.enabled }}
apiVersion: {{ required "A valid .Values.networkPolicyApiVersion entry required!" .Values.networkPolicyApiVersion }}

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

Why is the API version a values parameter?

@@ -0,0 +1,103 @@
{{- if include "nextcloud.host" . -}}
apiVersion: extensions/v1beta1

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

Can you use the apps/v1beta2 version instead?

@@ -0,0 +1,5 @@
dependencies:
- name: mariadb
version: 0.7.0

This comment has been minimized.

Copy link
@mattfarina

mattfarina Apr 26, 2018

Contributor

The version supports ranges. What about ~0.7.0 so that it can pull in patch level differences?

@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented May 2, 2018

Please compare with what you get from helm create using Helm 2.8+ and update accordingly.

@tcassaert tcassaert referenced this pull request May 16, 2018

Closed

Kubernetes Support #87

@pierreozoux

This comment has been minimized.

Copy link
Contributor

pierreozoux commented May 16, 2018

I think it would make more sense to PR against stable folder. It is a pretty straight forward addition.

Also I can help maintain this chart.

Another question: how could we keep this under /nextcloud github namespace, and sync it with the official charts? This would help a lot the pace of development.

@billimek

This comment has been minimized.

Copy link
Collaborator

billimek commented Jun 24, 2018

@affix are you still planning to finish this? I was about to write a chart for nextcloud as well, but will wait for this to complete - unless you want someone else to pick up where you left off?

@affix

This comment has been minimized.

Copy link
Contributor Author

affix commented Jun 24, 2018

@billimek I am planning to, just been a hectic few weeks. I will get it sorted and address comments

affix added some commits Jun 24, 2018

Changes from Code Review
- Change maintainer to affix
- Remove OWNERS file
- Comment out resources in values.yml
- Default to nextcloud 13.0.4-apache
- Specify Ingress api version
- Make Deployment v1beta2
- Support patch level MariaDB

Signed-off-by: Keiran Smith <contact@keiran.scot>
@affix

This comment has been minimized.

Copy link
Contributor Author

affix commented Jun 24, 2018

@mattfarina I have made the comments you requested

affix added some commits Jun 24, 2018

Changes from Code Review
- Change maintainer to affix
- Remove OWNERS file
- Comment out resources in values.yml
- Default to nextcloud 13.0.4-apache
- Specify Ingress api version
- Make Deployment v1beta2
- Support patch level MariaDB

Signed-off-by: Keiran Smith <contact@keiran.scot>
remove network policy values
Signed-off-by: Keiran Smith <contact@keiran.scot>
@unguiculus
Copy link
Member

unguiculus left a comment

Again, please compare with what you get from helm create using Helm 2.8+ and update accordingly (_helpers.tpl, standard labels, etc.).

Also, you'll need to merge in the latest master to make CI happy.


## Prerequisites

- Kubernetes 1.4+ with Beta APIs enabled

This comment has been minimized.

Copy link
@unguiculus

unguiculus Jun 25, 2018

Member

Should be 1.9+

@@ -0,0 +1,103 @@
{{- if include "nextcloud.host" . -}}
apiVersion: extensions/v1beta1

This comment has been minimized.

Copy link
@unguiculus

unguiculus Jun 25, 2018

Member

apps/v1

@@ -0,0 +1 @@
.git

This comment has been minimized.

Copy link
@unguiculus

unguiculus Jun 25, 2018

Member

Use .helmignore you get from helm create

# - secretName: nextcloud.cluster.local
# hosts:
# - nextcloud.cluster.local

This comment has been minimized.

Copy link
@unguiculus

unguiculus Jun 25, 2018

Member

I'd suggest you add one level of nesting:

nextcloud:
  host:
  loadBalancerIP:
  username:
  password:
  ...
@billimek

This comment has been minimized.

Copy link
Collaborator

billimek commented Jun 25, 2018

In the values.yaml there are references to,

persistence:
  enabled: true
  nextcloud:
    # storageClass: "-"
    # existingClaim:
    accessMode: ReadWriteOnce
    size: 8Gi

... however it doesn't appear that anything is referencing these values. From what I gather, this should be referencing where the nextcloud data itself resides. Perhaps the following should be added:

in deployment.yaml:

        resources:
{{ toYaml .Values.resources | indent 10 }}
        volumeMounts:
        - name: apache-data
          mountPath: /var/www/html
        - name: nextcloud-data
          mountPath: /var/www/html/data
      volumes:
      - name: apache-data
      {{- if .Values.persistence.apache.enabled }}
        persistentVolumeClaim:
          claimName: {{ if .Values.persistence.apache.existingClaim }}{{ .Values.persistence.apache.existingClaim }}{{- else }}{{ template "nextcloud.fullname" . }}-apache{{- end }}
      {{- else }}
        emptyDir: {}
      {{- end }}
      - name: nextcloud-data
      {{- if .Values.persistence.nextcloud.enabled }}
        persistentVolumeClaim:
          claimName: {{ if .Values.persistence.nextcloud.existingClaim }}{{ .Values.persistence.nextcloud.existingClaim }}{{- else }}{{ template "nextcloud.fullname" . }}-nextcloud{{- end }}
      {{- else }}
        emptyDir: {}
      {{- end }}

(see new references to nextcloud-data)

A new file, nextcloud-pvc.yaml created containing,

{{- if .Values.persistence.nextcloud.enabled -}}
{{- if not .Values.persistence.nextcloud.existingClaim -}}
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
  name: {{ template "nextcloud.fullname" . }}-nextcloud
spec:
  accessModes:
    - {{ .Values.persistence.nextcloud.accessMode | quote }}
  resources:
    requests:
      storage: {{ .Values.persistence.nextcloud.size | quote }}
{{- if .Values.persistence.nextcloud.storageClass }}
{{- if (eq "-" .Values.persistence.nextcloud.storageClass) }}
  storageClassName: ""
{{- else }}
  storageClassName: "{{ .Values.persistence.nextcloud.storageClass }}"
{{- end }}
{{- end }}
{{- end -}}
{{- end -}}

Also suggest moving the enabled stanza inside the respective apache and nextcloud sections so that users may choose which of the two (apache and/or nextcloud data) they want to enable.

@int128

This comment has been minimized.

Copy link
Contributor

int128 commented Jun 27, 2018

@affix Just FYI,
I have yet another Nextcloud chart using in our team.
https://github.com/int128/devops-kompose/tree/master/nextcloud
If you don't mind me, I am happy to contribute it and help you.

@billimek

This comment has been minimized.

Copy link
Collaborator

billimek commented Jun 27, 2018

The following also needs to change in order for the nextcloud autoconfiguration to work properly,

        - name: nextcloud_DATABASE_NAME
          value: {{ .Values.mariadb.mariadbDatabase | quote }}
        - name: nextcloud_DATABASE_USER
          value: {{ .Values.mariadb.mariadbUser | quote }}
        - name: nextcloud_DATABASE_PASSWORD

should be:

        - name: MYSQL_DATABASE
          value: {{ .Values.mariadb.mariadbDatabase | quote }}
        - name: MYSQL_USER
          value: {{ .Values.mariadb.mariadbUser | quote }}
        - name: MYSQL_PASSWORD
@billimek

This comment has been minimized.

Copy link
Collaborator

billimek commented Jun 27, 2018

affix#1 created with the changes I suggested.

Also added the ability to set existing claims for use in NextCloud via the 'external storage' plugin.

@@ -0,0 +1,19 @@
name: nextcloud
version: 0.1.1

This comment has been minimized.

Copy link
@giacomoguiulfo

giacomoguiulfo Oct 15, 2018

Collaborator

So apparently, charts going to stable need to start at 1.0.0

This comment has been minimized.

Copy link
@desaintmartin

desaintmartin Jan 15, 2019

Collaborator
Suggested change
version: 0.1.1
version: 1.0.0
@anarcat
Copy link

anarcat left a comment

so the bitnami docker image is gone, as others have noted here. i've tried to fix all the 404s refering to that image and point instead at the official docker image managed by the nextcloud organization. i am not sure it's sufficient here because the newer image might not work the same way esp. WRT the mariadb bitnami image.

@@ -0,0 +1,128 @@
# nextcloud

[nextcloud](https://nextcloud.org/) is a file sharing server that puts the control and security of your own data back into your hands.

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

making this formal with GH's new proposed change gizmo:

Suggested change
[nextcloud](https://nextcloud.org/) is a file sharing server that puts the control and security of your own data back into your hands.
[nextcloud](https://nextcloud.com/) is a file sharing server that puts the control and security of your own data back into your hands.
@@ -0,0 +1,170 @@
## Bitnami nextcloud image version
## ref: https://hub.docker.com/r/bitnami/nextcloud/tags/

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

Suggested change
## ref: https://hub.docker.com/r/bitnami/nextcloud/tags/
## Official nextcloud image version
## ref: https://hub.docker.com/r/library/nextcloud/tags/
email: user@example.com

## Set to `yes` to allow the container to be started with blank passwords
## ref: https://github.com/bitnami/bitnami-docker-nextcloud#environment-variables

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

Suggested change
## ref: https://github.com/bitnami/bitnami-docker-nextcloud#environment-variables
## ref: https://github.com/nextcloud/docker#auto-configuration-via-environment-variables
enabled: true

## Create a database
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-on-first-run

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

another broken link:

Suggested change
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-on-first-run
## ref: https://github.com/nextcloud/docker#using-an-external-database
mariadbDatabase: nextcloud

## Create a database user
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-user-on-first-run

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

just remove this duplicate link

Suggested change
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-user-on-first-run
mariadbUser: nextcloud

## Password for mariadbUser
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-user-on-first-run

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

just remove this duplicate link

Suggested change
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#creating-a-database-user-on-first-run
# mariadbPassword:

## MariaDB admin password
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#setting-the-root-password-on-first-run

This comment has been minimized.

Copy link
@anarcat

anarcat Oct 24, 2018

just remove this duplicate link

Suggested change
## ref: https://github.com/bitnami/bitnami-docker-mariadb/blob/master/README.md#setting-the-root-password-on-first-run
@varac

This comment has been minimized.

Copy link

varac commented Nov 21, 2018

Hej, what's the state of this ? Would be great to have it meged soon!

@iMartyn

This comment has been minimized.

Copy link
Contributor

iMartyn commented Nov 22, 2018

FYI the official nextcloud docker image lacks php-imap support because a maintainer deemed it to be "too big". This makes it useless for my setup, even if the helm chart works. :'(

@anarcat

This comment has been minimized.

Copy link

anarcat commented Nov 22, 2018

@kfox1111

This comment has been minimized.

Copy link
Contributor

kfox1111 commented Dec 13, 2018

Any updates?

@joshuacox

This comment has been minimized.

Copy link

joshuacox commented Dec 27, 2018

+1 on getting this merged

@doodlemania2

This comment has been minimized.

Copy link

doodlemania2 commented Dec 31, 2018

Pretty please?

@kfox1111

This comment has been minimized.

Copy link
Contributor

kfox1111 commented Dec 31, 2018

If we can get something merged, then it will be easier for people to contribute bugfixes/features.

@joshuacox

This comment has been minimized.

Copy link

joshuacox commented Dec 31, 2018

perhaps @affix abandoned this PR? What is left from @unguiculus review that is undone? I agree with @kfox1111 let's wrap this up and get it merged so it can get some wider testing and then new PRs.

@ngortheone

This comment has been minimized.

Copy link

ngortheone commented Jan 14, 2019

Also interested in this chart. I think it is reasonable to merge this to incubating so then interested people can contribute fixes

@@ -0,0 +1,19 @@
name: nextcloud
version: 0.1.1

This comment has been minimized.

Copy link
@desaintmartin

desaintmartin Jan 15, 2019

Collaborator
Suggested change
version: 0.1.1
version: 1.0.0
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 15, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: affix
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattfarina

If they are not already assigned, you can assign the PR to them by writing /assign @mattfarina in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@anarcat

This comment has been minimized.

Copy link

anarcat commented Jan 16, 2019

if someone is really interested in getting this off the ground, the way forward is to just make a new PR, because the proposed changes here do not seem to be going anywhere...

@guglez

This comment has been minimized.

Copy link

guglez commented Jan 23, 2019

/assign @mattfarina

@EmperorArthur

This comment has been minimized.

Copy link

EmperorArthur commented Jan 24, 2019

Came looking for a Nextcloud chart and found this PR. I see a few things haven't been fixed from the changes requested. I'd fix it, but am still learning. Hope it's still being actively worked on.

PS: The Details button on "tide" goes to a 404.

@chrisingenhaag chrisingenhaag referenced this pull request Jan 26, 2019

Merged

add nextcloud chart #10922

3 of 3 tasks complete
@chrisingenhaag

This comment has been minimized.

Copy link
Contributor

chrisingenhaag commented Jan 27, 2019

Hey together,
I created a new, bit refactored and updated the chart. PR is #10922 . I hope I´ve got all of your feedback and that we can get this chart into stable.

chrisingenhaag added a commit to chrisingenhaag/charts that referenced this pull request Jan 27, 2019

insert suggestions from reviews in helm#5180
Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>
@evanstucker-hates-2fa

This comment has been minimized.

Copy link

evanstucker-hates-2fa commented Feb 5, 2019

I'm looking forward to this getting merged.

@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented Feb 8, 2019

I'd suggest we move forward with #10922.

k8s-ci-robot added a commit that referenced this pull request Feb 11, 2019

add nextcloud chart (#10922)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in #5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>
@unguiculus

This comment has been minimized.

Copy link
Member

unguiculus commented Feb 11, 2019

Closing in favor of #10922.

@unguiculus unguiculus closed this Feb 11, 2019

k8s-ci-robot added a commit that referenced this pull request Feb 12, 2019

Nextcloud chart add OWNERS for further contribution (#11335)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in #5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* [nextcloud] add owners file for further contribution

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

tbuchier added a commit to tbuchier/charts that referenced this pull request Feb 14, 2019

add nextcloud chart (helm#10922)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in helm#5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

tbuchier added a commit to tbuchier/charts that referenced this pull request Feb 14, 2019

Nextcloud chart add OWNERS for further contribution (helm#11335)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in helm#5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* [nextcloud] add owners file for further contribution

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

Dariusch pushed a commit to Dariusch/charts that referenced this pull request Mar 4, 2019

add nextcloud chart (helm#10922)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in helm#5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

Dariusch pushed a commit to Dariusch/charts that referenced this pull request Mar 4, 2019

Nextcloud chart add OWNERS for further contribution (helm#11335)
* add nextcloud chart

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* insert suggestions from reviews in helm#5180

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* disable ingress per default

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* fix nextcloud e2e tests

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>

* [nextcloud] add owners file for further contribution

Signed-off-by: Christian Ingenhaag <christian.ingenhaag@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.