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

Re-write it in Go #35

Merged
merged 33 commits into from
Nov 7, 2018
Merged

Re-write it in Go #35

merged 33 commits into from
Nov 7, 2018

Conversation

unguiculus
Copy link
Member

I built a first beta release on my own fork. For this initial release I put the focus on more or less keeping the same functionality, but configuration is much more flexible now. Once we get this merged, we can look at new features such as integrating additional tools like Kubeval or better reporting capabilities.

Please review and test.

https://github.com/unguiculus/chart-testing/releases/tag/v2.0.0-beta.0

@helm/charts-maintainers

@scottrigby
Copy link
Member

I've looked at this on your fork and it's great 👏 I plan to dig into this in more depth soon

For anyone who wants more context, see #29

@rimusz
Copy link
Contributor

rimusz commented Oct 22, 2018

awesome work @unguiculus 👍 I will give a go for test

@unguiculus
Copy link
Member Author

Here's a test PR that updates charts CI in order to demonstrate it works as expected:
helm/charts#8745

Copy link
Contributor

@rimusz rimusz left a comment

Choose a reason for hiding this comment

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

would be nice to put to readme flags for lint, install, lint-and-install:

$ ./ct lint-and-install -h

Combines 'lint' and 'install' commands.

Usage:
  ct lint-and-install [flags]

Flags:
      --all                        Process all charts except those explicitly excluded.
                                   Disables changed charts detection and version increment checking
      --build-id string            An optional, arbitrary identifier that is added to the name of the namespace a
                                   chart is installed into. In a CI environment, this could be the build number or
                                   the ID of a pull request. If not specified, the name of the chart is used.
      --chart-dirs strings         Directories containing Helm charts (default [charts])
      --chart-repos strings        Additional chart repos to add so dependencies can be resolved
      --chart-yaml-schema string   The schema for chart.yml validation. If not specified, 'chart_schema.yaml'
                                   is searched in '/etc/ct', '$HOME/ct' and the current directory
      --charts strings             Specific charts to test.
                                   Disables changed charts detection and version increment checking
      --check-version-increment    Activates a check for chart version increments (default: true) (default true)
      --config string              Config file
      --excluded-charts strings    Charts that should be skipped
  -h, --help                       help for lint-and-install
      --lint-conf string           The config file for YAML linting. If not specified, 'lintconf.yaml' is
                                   searched in '/etc/ct', '$HOME/ct' and the current directory
      --remote string              The name of the Git remote used to identify changed charts (default "origin")
      --target-branch string       The name of the target branch used to identify changed charts (default "master")
      --tiller-namespace string    The namespace of Tiller (default: kube-system) (default "kube-system")
      --timeout duration           The timeout for Helm in seconds (default: 5m) (default 5m0s)
      --validate-maintainers       Enabled validation of maintainer account names in chart.yml (default: true).
                                   Works for GitHub, GitLab, and Bitbucket (default true)

So you do. to have to run commands yourself to check that out.

@unguiculus
Copy link
Member Author

@rimusz I wasn't sure whether we should do that. I thought that might be too much. What does everyone else think?

@rimusz
Copy link
Contributor

rimusz commented Oct 25, 2018

e.g. to add a folder docs and there to store more detailed info.

Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

Nice job on the re-write. I like the direction. On the whole it LGTM. Because of the size I gave it a relatively fast review (~1 hour) for the size. There is a lot here.

app/cmd/lintAndInstall.go Outdated Show resolved Hide resolved
app/cmd/lintAndInstall.go Outdated Show resolved Hide resolved
examples/gke/my_test.sh Outdated Show resolved Hide resolved
examples/gke/my_test.sh Outdated Show resolved Hide resolved
CheckVersionIncrement bool `mapstructure:"check-version-increment"`
ProcessAllCharts bool `mapstructure:"all"`
Charts []string `mapstructure:"charts"`
ChartRepos []string `mapstructure:"chart-repos"`
Copy link
Collaborator

@mattfarina mattfarina Oct 30, 2018

Choose a reason for hiding this comment

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

I see for the format for ChartRepos is incubator=https://incubator. Would it be more intuitive if those were broken up? Then the YAML would look something like:

chart-repos:
  - name: incubator
    location: https://incubator

Copy link
Member Author

Choose a reason for hiding this comment

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

All Cobra CLI flags are bound to Viper. This is generic. I guess changing this is not easily possible. And it would not be consistent.

OWNERS Outdated Show resolved Hide resolved
@helm-bot helm-bot added size/XXL and removed size/XXL labels Nov 6, 2018
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
This reverts commit fcbfbdc.

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
@helm-bot helm-bot added size/XXL and removed size/XXL labels Nov 6, 2018
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
@helm-bot helm-bot added size/XXL and removed size/XXL labels Nov 6, 2018
@unguiculus
Copy link
Member Author

@scottrigby
Copy link
Member

scottrigby commented Nov 6, 2018

#35 (comment) and #35 (comment) have been addressed. README has been clarified on that point. File inclusion looks good in the new beta now too 💯

$ tree
.
├── LICENSE
├── README.md
├── ct
└── etc
    ├── chart_schema.yaml
    └── lintconf.yaml

1 directory, 5 files

Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
@helm-bot helm-bot added size/XXL and removed size/XXL labels Nov 6, 2018
Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

This looks good to merge. Any future changes or fixes can come in smaller PRs.

Nicely done @unguiculus. This rewrite was a massive undertaking and you make it look easier than I could have.

Copy link
Member

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

I had a very quick look through, and it's looking pretty solid to me. I'm not sure if you considered it, but we could import the Helm API for some of the structs we're using rather than duplicate them here, but that's a minor issue and we may want to have control over those structs here.

pkg/util/util.go Show resolved Hide resolved
@scottrigby
Copy link
Member

We've had all eyes on this, and it has been tested extensively now. Merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants