I have tested this chart with all the provided options and in terms of database i used internal and MySQL.

Corresponding documentation update : https://code.onedev.io/onedev/docs/~pulls/14

issue onedev/server#1421

Thanks for your contribution. Really appreciated!

I started a new gke cluster, and the initial installation gives me below error:

Error: INSTALLATION FAILED: failed to create resource: PersistentVolumeClaim "onedev" is invalid: spec.accessModes: Required value: at least 1 access mode is required

This is how I tested:

$ cd server
$ mvn clean package
$ cd server-product/helm
$ ./build.sh
$ cd ../target/helm-chart/onedev
$ helm install onedev . --namespace onedev --create-namespace

Also I noticed that letsencrypt setup has been removed. Any reason for that?

Some more findings:

1. Originally there is option to separate ssh and http service (check pull request #32 for reason), but it is not possible now
2. Some settings in values.yaml should never be changed by users. These settings include volume mount point (/opt/onedev), command and args. Changing them will break OneDev. My understanding is that values.yaml should only include settings allowed to be changed by users

Issues are fixed now, it was an oversight on my part , the accessModes option got removed in pvc.

It makes sens to have separate ssh service based on the info pull request onedev/server#32 here. I have added the manifest for separate ssh service and config params in values.yaml to install ssh service separately if enabled.

Following parameters have been removed from values.yaml and not configurable any more. Values are set in deployment manifest.

• command , args, container port overwrite , mountPath

In my opinion, it would be better if the Let's Encrypt issuer is not included in the chart deployment. The primary focus of the chart should be solely on OneDev. If a user has cert-manager set up, they can easily add an issuer manifest separately.

It's important to note that the chart assumes the presence of cert-manager, if enabled in chart, it can cause installation failures for users without cert-manager.

It would be more beneficial to add the issuer manifest to the documentation instead of embedding it in the chart. This way, users can refer to the documentation for guidance on how to apply the issuer manifest, and it ensures that the chart remains focused on OneDev installation.

Installation docs have been updated with these instruction and issuer manifest.

That is just my opinion , if still needed let me know i can add it back and push the changes.

Issues are fixed now, it was an oversight on my part , the accessModes option got removed in pvc.

I tested and now it works!

In my opinion, it would be better if the Let's Encrypt issuer is not included in the chart deployment. The primary focus of the chart should be solely on OneDev. If a user has cert-manager set up, they can easily add an issuer manifest separately.

Totally agree with you. As long as there is a step by step guide for setting up letsencrypt, that will be fine. Thanks for adding guide for that. Will test it later.

Now I can deploy the chart into a brand new cluster and access OneDev via localhost through port forwarding, which is great. I continue to test with ingress setup following the guide. One problem I am encountering is that I maintain dns records out of k8s at godaddy. Currently there is no instructions on how to get the external ip for OneDev service.

Some more suggestions:

1. OneDev web service can only be accessed via "/", mounting to subpath will lead to problems. So I'd suggest to remove path specific settings below:

   hosts:
   # ingress.hosts.host -- Set the host name
    - host: onedev.example.com
      paths:
        - path: /
          pathType: ImplementationSpecific
          # Specify the port name or number on the Service
          # Using name requires Kubernetes >=1.19
          port:
            name: "http"
            number: ""    
   

2. Section name oneDevServer seems a little verbose, can we just change it to onedev?

Path specific settings are now part of ingress resources after version 1.18 Changelog of k8s. We are just providing option for users to change this settings from values if they have a different setup. Latest Ingress Docs.

Lets say a user have setup in a subpath example.com/onedev , they can change in values to reflect that

hosts:
 - host: example.com
   paths:
     - path: /onedev
       pathType: Prefix

The default settings are provided keeping majority of people like me who will setup the service at subdomain onedev.example.com

One problem I am encountering is that I maintain dns records out of k8s at godaddy. Currently there is no instructions on how to get the external ip for OneDev service.

Can you provide me a bit more details about this , We already have some instructions in NOTES.txt which will be displayed if you are using service type as Loadbalancer, i just need to understand if this is different then that

Section name oneDevServer seems a little verbose, can we just change it to onedev?

Yes sure, i will make the changes

One big concern is for existing k8s deployment. Following upgrade guide will certainly no longer works, as original version uses a mysql database. Any possiblity for a smooth upgrade?

Lets say a user want have setup in a subpath example.com/onedev , they can change in values to reflect that

It is not possible for OneDev to be accessed via a subpath like this, it only works for top level path like onedev.example.com

This is because OneDev has some internal routing logic which does not play well with subpath.

One big concern is for existing k8s deployment. Following upgrade guide will certainly no longer works, as original version uses a mysql database. Any possiblity for a smooth upgrade?

One option is to enable the external database options and provide the MySQL database details, this way users will continue to use that database after upgrade. This will work great for users who are using already using MySQL and deployed it separately.

For the users who deployed it as part of the chart , have a couple of options

1. Stop the service, redeploy MySQL and use the same volume, this way they don't lose data and then provide MySQL db details in the database sections.
2. Get the MySQL manifest already generated by the current chart and save it to a file , remove all the helm labels from it and apply with kubectl this way MySQL will not be considered as part of this chart when doing helm upgrade. And users can enable external dbs and provide values there.

This can be provided in upgrade section of the docs.

I am not sure how onedev handles db migrations or if that is even an option but if there is a way you can mention that

Lets say a user want have setup in a subpath example.com/onedev , they can change in values to reflect that

It is not possible for OneDev to be accessed via a subpath like this, it only works for top level path like onedev.example.com

I will make those changes for ingress

I have pushed new changes

• Path and pathType prams have been removed from values file and set in ingress manifest.
• oneDevServer section has been renamed to onedev and all references in templates have been updated.

For the users who deployed it as part of the chart , have a couple of options

Stop the service, redeploy MySQL and use the same volume, this way they don't lose data and then provide MySQL db details in the database sections. Get the MySQL manifest already generated by the current chart and save it to a file , remove all the helm labels from it and apply with kubectl this way MySQL will not be considered as part of this chart when doing helm upgrade. And users can enable external dbs and provide values there. This can be provided in upgrade section of the docs.

In original version, mysql runs in same pod as onedev server and this is the only option. It is reasonable to ask user to deploy mysql separately and have them providing db details to onedev. In case user just follow the old one-liner upgrade guide from original chart, can we print some warning in the chart refusing to upgrade and pointing them to the new upgrade guide? I am not a helm expert, so want to listen to your suggestions.

Once onedev gets connected to mysql, it can handle data upgrade in place without any issues.

Now I can deploy the chart into a brand new cluster and access OneDev via localhost through port forwarding, which is great. I continue to test with ingress setup following the guide. One problem I am encountering is that I maintain dns records out of k8s at godaddy. Currently there is no instructions on how to get the external ip for OneDev service.

Any idea for this?

In original version, mysql runs in same pod as onedev server and this is the only option. It is reasonable to ask user to deploy mysql separately and have them providing db details to onedev. In case user just follow the old one-liner upgrade guide from original chart, can we print some warning in the chart refusing to upgrade and pointing them to the new upgrade guide? I am not a helm expert, so want to listen to your suggestions.

Direct helm upgrade is not possible if users have older version installed in cluster before. They will run into install errors, this chart is greatly different then existing deployment. So the only option for users is to refer to documentation. Also this prevents users from doing accidental upgrades and losing data. We can can display warning in the install guide and point them to upgrade guide, i will add documentation for doing upgrades if users are coming from older versions of chart if they did install with one liner and mysql was installed as part of chart.

Any idea for this?

When you use service type as "LoadBalancer", upon install the chart will display information on how to get the loadbalancer IP

I am doing local deployment and already know the IP of LoadBalancer which i provide in service.loadBalancerIP. For cloud (aws/gcp) you can run the following kubectl command to get the IP

** Please ensure an external IP is associated to the onedev service before proceeding **
** Watch the status using: kubectl get svc --namespace onedev -w onedev **

  export SERVICE_IP=$(kubectl get svc --namespace onedev onedev --template "{{ range (index .status.loadBalancer.ingress 0) }}{{ . }}{{ end }}")

  echo "URL: http://$SERVICE_IP:6610/"

Here SERVICE_IP is your loadbalancer IP, and this information is only displayed if service type is Loadbalancer.

In the second scenario, if you are using the "ClusterIP" service type and planning to enable ingress in your deployment, Your DNS record is set up with providers like GoDaddy or Route53, You already know the IP of your existing loadbalancer and that is what you are adding in DNS A record.

That is if i understood the question correctly

Information about upgrading from an older version of chart is now a part of documentation, and note has also been added in deploy section with link to upgrades.

When you use service type as "LoadBalancer", upon install the chart will display information on how to get the loadbalancer IP

I am trying to change service type to LoadBalancer as below:

helm upgrade onedev . --namespace onedev --set service.type=LoadBalancer --reuse-values

And GKE reports below error:

Error: UPGRADE FAILED: cannot patch "onedev" with kind PersistentVolumeClaim: PersistentVolumeClaim "onedev" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests for bound claims
  core.PersistentVolumeClaimSpec{
  	... // 2 identical fields
  	Resources:        {Requests: {s"storage": {i: {...}, s: "10Gi", Format: "BinarySI"}}},
  	VolumeName:       "pvc-e3076b24-ec1a-4ede-9def-c943ee46b743",
- 	StorageClassName: &"standard-rwo",
+ 	StorageClassName: nil,
  	VolumeMode:       &"Filesystem",
  	DataSource:       nil,
  	DataSourceRef:    nil,
  }

Nevertheless, the load balancer does get created, with an external ip

I think you are trying to updated the deployment from older chart, as i mention it won't be possible to upgrade from older charts

No, I am not upgrading the old chart. Just installed the new chart, and then add a load balancer.

I will test it and get back

The error is a known helm issue when no storageclass name is provided, its fixed now. As for the loadbalancer it works for me, i am not using any cloud provider but it should work with provider as well. Let me know how the tests goes.

Works as expected now.

One minor improvement: when I set service to LoadBalancer after chart is deployed, the welcome message tells me to access OneDev via url onedev.example.com even if I did not enable ingress. If I use LoadBalancer for initial chart install, the welcome message is correct (telling me how to get the ip address).

Another question: I am trying to use one-liner to achieve some typical use cases, for instance, to enable ingress and use custom host name I am using:

helm upgrade onedev . --namespace onedev --set ingress.enabled=true --set 'ingress.hosts[0].host=test.onedev.io' --reuse-values

However it gives me error:

Error: UPGRADE FAILED: cannot patch "onedev" with kind Ingress: Ingress.extensions "onedev" is invalid: spec.rules[0].http.paths: Required value

Seems that paths are not picked up from values.yaml for hosts[0], but I am only overriding the host part. Or I misunderstand things here?

One minor improvement: when I set service to LoadBalancer after chart is deployed, the welcome message tells me to access OneDev via url onedev.example.com even if I did not enable ingress. If I use LoadBalancer for initial chart install, the welcome message is correct (telling me how to get the ip address).

Maybe you you have ingress enabled accidentally, as you can see from my screenshot above, when i changed to LoadBalancer it does not show ingress.

Seems that paths are not picked up from values.yaml for hosts[0], but I am only overriding the host part. Or I misunderstand things here?

You are right about this, when i hard-coded the path prefix there where some conditions which made it harder for this to work. The issue is now fixed and only one host is supported like current chart.

Use below to test it

helm upgrade onedev . --namespace onedev --set ingress.enabled=true --set 'ingress.host=test.onedev.io' --reuse-values

Maybe you you have ingress enabled accidentally, as you can see from my screenshot above, when i changed to LoadBalancer it does not show ingress.

I tested again and the message is displaying correctly. Guess I've enabled ingress unintentionally. Sorr for that.

Now the problem is that when I enable LoadBalancer, it does allocate an external ip, but OneDev can only be accessed via http://<external ip>:6610. It should be access via port 80 instead.

You are right about this, when i hard-coded the path prefix there where some conditions which made it harder for this to work. The issue is now fixed and only one host is supported like current chart.

Works as expected now. Thx! Only problem is that it should be accessed via port 80 instead of 6610 (see above comment for LoadBalancer).

Service is set to the default port of 6610 where onedev exposes http traffic, when none is provided.

You can easily set to port 80 or any other port you like by either setting it in the values file service.ports.httpPort or at deployment time.

helm install . --set service.ports.httpPort=80

This extra flexiblity is fine. I think the welcome message needs to be tuned according to current active port:

1. You can access OneDev at:

   http://onedev.example.com

The same for message when ingress is not enabled

Yes , its has been changed to display the port which is set in values or use default.

1. You can access OneDev at:

http://onedev.example.com

This is always going to be 80,443 when ingress is enabled which is handled by ingress controller.

This is always going to be 80,443 when ingress is enabled which is handled by ingress controller.

I see.

There is http port setting for ingress section. I think this is always the same as service port. Can we remove this setting and use service port directly in ingress template?

Removing this setting does not offer any advantages or benefits. Firstly, it offers flexibility in configuration options.

Secondly, our ingress generates a manifest for multiple Kubernetes versions, based on the user's installed version. The logic for generating the manifest relies on this specific settings to determine whether a port number or name is provided.

Removing this setting does not offer any advantages or benefits. Firstly, it offers flexibility in configuration options.

Every time you change the service port, you will need to change the port in ingress section also, which is not DRY. I hope that OneDev is easy to configure and maintain

Secondly, our ingress generates a manifest for multiple Kubernetes versions, based on the user's installed version. The logic for generating the manifest relies on this specific settings to determine whether a port number or name is provided

Guess you may consult the service port instead?

Every time you change the service port, you will need to change the port in ingress section, which is not DRY. I hope that OneDev is easy to configure and maintain

For this reason we have port name , if you change the service port, the name of port is always same http so you don't have to change in ingress.

Guess you may consult the service port instead?

Alright then, changed to service port

Thanks for making this change. I really like what the new helm chart looks like now. Flexible while still easy to configure for typical use cases. Will continue to test other usage scenarios, so bear with my verbosity. 😊

You're welcome!

Indeed, it is good practice to thoroughly test everything at this stage to ensure comprehensive coverage and address all possible scenarios.

I merged ths pull request, and will make some adjustments based on your work. Will summarize my changes when done.

I merged ths pull request, and will make some adjustments based on your work. Will summarize my changes when done.

Well noted

Changes made by me:

https://code.onedev.io/onedev/server/~commits?query=since+commit%28a8b1ecb11cb6a9f0fe9acf35a36a33d084248ccc%29+until+commit%28dd7f91e3e4463c06172788451340eea1d0b013b3%29

Will continue to update docs to reflect the change. Let me know if you have any concerns.