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

Kubeapps chart - Lint chart #1299

Merged
merged 3 commits into from
Nov 20, 2019

Conversation

juan131
Copy link
Contributor

@juan131 juan131 commented Nov 19, 2019

Signed-off-by: juan131 juan@bitnami.com

Description of the change

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Thanks Juan. From your description (and looking at some of the changes) this looks like a combination of a mechanical change and some additions. Just for next time, it'd be great to put the (large) mechanical change (ie. linting, formatting, but no actual change) in one diff - as it doesn't really require a review - with the actual content changes in a small, easy to read one.

On another note, do you think it would be worth (separately) updating the checks on a PR so that it fails if the yaml/helm lint produce any changes, stopping a PR from being merged?
Thanks for the im

@juan131
Copy link
Contributor Author

juan131 commented Nov 20, 2019

From your description (and looking at some of the changes) this looks like a combination of a mechanical change and some additions.

Yes, it's a combination of "linting changes" and changes to standardise the manifests, so they're consistent with other Bitnami charts.

I can split the diff if you it's very confused for you to review it.

do you think it would be worth (separately) updating the checks on a PR so that it fails if the yaml/helm lint produce any changes, stopping a PR from being merged?

Yes, I think it can be a great addition. But let's wait until we merge on bitami/charts repo the "final" version of the scripts we want to use to test the charts.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

chart/kubeapps/values.yaml Show resolved Hide resolved
Comment on lines 271 to 281
## It's possible to modify the default timeout for install/upgrade/rollback/delete apps
## (Default: 300s)
# timeout: 300
## Tiller Proxy containers' resource requests and limits
## ref: http://kubernetes.io/docs/user-guide/compute-resources/
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment is now a bit confusing (because its new place)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean this one?

  ## TLS parameters
  tls: {}
  #  ca:
  #  cert:
  #  key:
  #  verify: false

Or nesting the comments with the ones for other parameters (e.g. timeout, resources)?

chart/kubeapps/Chart.yaml Outdated Show resolved Hide resolved
@absoludity
Copy link
Contributor

I can split the diff if you it's very confused for you to review it.

No no :) It's not worth the effort now, I just meant for next time (it's much closer to zero cost at the time). Thanks!

juan131 added 2 commits November 20, 2019 17:02
Signed-off-by: juan131 <juan@bitnami.com>
Signed-off-by: juan131 <juan@bitnami.com>
@juan131 juan131 force-pushed the lint-kubeapps-chart branch from d48d202 to 0717f55 Compare November 20, 2019 16:02
Signed-off-by: juan131 <juan@bitnami.com>
@andresmgot andresmgot merged commit f480eca into vmware-tanzu:master Nov 20, 2019
@juan131 juan131 deleted the lint-kubeapps-chart branch November 21, 2019 08:48
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.

3 participants