-
Notifications
You must be signed in to change notification settings - Fork 16.8k
Conversation
/retest |
@cdrage it looks like you need to re-create the requirements.lock file. That's the CI issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I did a quick review and have a little feedback.
incubator/gitea/Chart.yaml
Outdated
description: 'Gitea: Go Git Service' | ||
name: gitea | ||
version: 1.3.2 | ||
appVersion: 0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you have the version
and appVersion
switched. The version
is the version of the chart or, rather, the configuration to deploy to Kubernetes. The appVersion
is the version of the underlying application being deployed.
incubator/gitea/Chart.yaml
Outdated
- git | ||
- gitea | ||
maintainers: | ||
- name: Charlie Drage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of your name can you please put your github username here. That will make it easier for us to contact you in continued maintenance of the chart.
incubator/gitea/requirements.yaml
Outdated
@@ -0,0 +1,5 @@ | |||
dependencies: | |||
- name: postgresql | |||
version: 0.8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a SemVer range for the version. You can read about the details at https://github.com/Masterminds/semver#basic-comparisons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mattfarina I think you're wrong. A lot of the stable charts use the exact same dependency with 0.8.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version of that chart is 0.8.9
which means there have been 8 patch releases you'll not get. These include bug fixes and minor improvements.
In the chart best practices guide, in the helm docs, it recommends using ranges.
There are a number of charts here that aren't doing that. We likely need to make fixes to those other charts to clean up quality. PRs are welcome.
@@ -0,0 +1,64 @@ | |||
apiVersion: extensions/v1beta1 | |||
kind: Deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Kubernetes 1.10 this API version can be dropped. Since this Deployments have gone to apps/v1beta1
, apps/v1beta2
, and most recently apps/v1
. In 1.10 anything prior to apps/v1beta2
can be dropped. This is the version I would recommend specifying unless you want to support multiple versions by checking in Capabilities.APIVersions for a version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment, sorry! I'm following the other stable charts as well for motivation. Seems to be in-line with how the others do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add some more context, there is a deprecation policy page with details and examples. Based on the timing, this API has not been removed due to a bug.
A bunch of the charts were created with extensions/v1beta1
. One of the TODOs we need to tackle for the charts repo is updating them before the API version goes away. To have less work when that happens we've been asking charts to use a newer API version.
spec: | ||
type: {{ .Values.serviceType }} | ||
ports: | ||
- port: {{ .Values.service.httpPort | int }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some files the list indentation is 0 and in other files it's 2 spaces. Can you please make it consistent across the chart at a 2 space indentation.
/retest |
@mattfarina Thanks so much for the review. I updated the last two sections (version switched to |
Can we maybe add Also looks like |
@filoozom Odd, I rebuilt it... didn't seem to work. |
Are you talking about $ helm dependency build
...Successfully got an update from the "incubator" chart repository
...Successfully got an update from the "stable" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading postgresql from repo https://kubernetes-charts.storage.googleapis.com/
Deleting outdated charts
$ cat requirements.lock
dependencies:
- name: postgresql
repository: https://kubernetes-charts.storage.googleapis.com/
version: 0.8.12
digest: sha256:fa8809a605bfa4b36a22ff37b10a7b94f36b227e4d2bca359e6a3a11b791dced
generated: 2018-02-20T14:54:32.069629245Z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also be nice to add service.gitea.disableSsh
(DISABLE_SSH
) too 😃
## | ||
cacheHost: | ||
|
||
## Enable this to use captcha validation for registration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong description message for serverDomain
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cdrage Assign the PR to them by writing 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 |
@mattfarina Can we get this merged so that I can work on extending this chart in a new PR? |
Yeah, sorry @filoozom been busy with a few other projects, but yes 👍 I'd be down to get this merged if @mattfarina has anything else to say about this |
/hold |
I'd like you to have a look at our review guidelines, especially regarding standard labels. |
/assign |
/assign @prydonius |
Ping @mattfarina Any updates? |
@cdrage See my comment above. We are actually waiting for an update from your side. Thanks. Feel free to ask if you have questions. |
Hi @unguiculus I did read your comment: If you don't mind helping within the code-review, that'd be great. I believe I'm simply missing the labels within Deployment? https://github.com/kubernetes/charts/pull/3408/files#diff-c701936730a96a12c3f3ca5a38e30228R6 |
ping @unguiculus @prydonius (I've already gone ahead and updated a few of the charts / templates with the afformentioned changes) |
/retest |
1 similar comment
/retest |
Going to set this as a [WIP]. Seems like I broke something but I'm a bit busy this week to test it out. |
@cdrage Guess it's because of this: go-gitea/gitea#3246 |
This is a follow up to PR's #2083 as well as #346 Gitea is a fork of Gogs and thus, (most) of this PR has been adapted from: https://github.com/kubernetes/charts/tree/master/incubator/gogs At the moment there is no Gitea chart available in the incubator. Unfortunately there is little activity on Gogs (see: https://github.com/gogits/gogs) since it's ran by only one maintainer (rather than a community).
@cdrage: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@filoozom Yeah... this sucks. We're blocked because of: go-gitea/gitea#3246 Why they use app.ini as a status, I do not know. |
I have a working chart in PR #6124, take a look and see if there are things we can merge here. I used an init container and an emptyDir shared volume to get around the INTERNAL_TOKEN issue. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Activing will cause the issue to no longer be considered stale. Thank you for your contributions. |
Is this still considered work in progress as the title suggests? |
@mattfarina blocked more like it, since we're waiting for an issue to be resolved from Gitea (go-gitea/gitea#3246) |
@cdrage Thanks for the update |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions. |
This issue is being automatically closed due to inactivity. |
As i understand the blocking issue is solved here go-gitea/gitea#5812 ? so maybe it's time to add it to incubator? |
Any chance to get gitea charts on incubator?? |
Yah, I would be very interested in a gitea helm chart. It looks like the blocking issue has been resolved. It would be great to get this into incubator! |
I can work on getting this back into the incubator, although with changes to Helm and Gitea I hope it'd work. |
This is a follow up to PR's
#2083 as well as #346
Gitea is a fork of Gogs and thus, (most) of this PR has been adapted
from: https://github.com/kubernetes/charts/tree/master/incubator/gogs
At the moment there is no Gitea chart available in the incubator.
Unfortunately there is little activity on Gogs (see:
https://github.com/gogits/gogs) since it's ran by only one maintainer
(rather than a community).