Closed kmarteaux closed 2 years ago
Gustav,
Thanks for the feedback. I will make the changes you've outlined and resubmit.
--Ken
On Wed, Nov 24, 2021 at 8:06 AM Gustav Westling @.***> wrote:
@.**** requested changes on this pull request.
Hey! Thanks for contributing to kube-score, this is a good check, thanks for adding it!
We don't want to add more flags to "kube-score score" unless absolutely necessary. The code that you've copy/pasted from is fairly old, so it was not the best start for you - sorry!
If you introduce this as a new check, instead of adding it to the existing one, kube-scores built-in --ignore-test functionality can be used to ignore the test instead of having to add a new flag. With the added bonus that the ignores can be done on a per object basis via annotations!
In score/score.go https://github.com/zegl/kube-score/pull/414#discussion_r756040413:
@@ -17,7 +17,6 @@ import ( "github.com/zegl/kube-score/score/service" "github.com/zegl/kube-score/score/stable" "github.com/zegl/kube-score/scorecard"
There is a lot of diffs that move around imports, could you please remove them, so that we're consistent with the way imports where sorted before.
In score/stable_version_test.go https://github.com/zegl/kube-score/pull/414#discussion_r756041150:
@@ -17,15 +17,15 @@ func TestStatefulSetAppsv1beta1Kubernetes1dot4(t *testing.T) { t.Parallel() testExpectedScoreWithConfig(t, config.Configuration{ AllFiles: []ks.NamedReader{testFile("statefulset-appsv1beta1.yaml")},
- KubernetesVersion: config.Semver{1, 4},
- KubernetesVersion: config.Semver{Major: 1, Minor: 4},
This is a unrelated change, could we keep it the way it was (alternatively move this to a separate Pull Request)? Thanks.
In cmd/kube-score/main.go https://github.com/zegl/kube-score/pull/414#discussion_r756046766:
@@ -99,6 +98,7 @@ func scoreFiles(binName string, args []string) error { exitOneOnWarning := fs.Bool("exit-one-on-warning", false, "Exit with code 1 in case of warnings") ignoreContainerCpuLimit := fs.Bool("ignore-container-cpu-limit", false, "Disables the requirement of setting a container CPU limit") ignoreContainerMemoryLimit := fs.Bool("ignore-container-memory-limit", false, "Disables the requirement of setting a container memory limit")
- ignoreContainerStorageEphemeralLimit := fs.Bool("ignore-container-ephemeral-storage-limit", false, "Disables the requirement of setting a container ephemeral-storage limit")
kube-score has a better mechanism for ignoring checks these days, via the --ignore-test flag and kube-score/ignore annotation.
Could you use that mechanism instead? This means that you will have to add this feature as a new check, instead of adding it to Container Resources, but I think that's a good trade-off.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/zegl/kube-score/pull/414#pullrequestreview-814864766, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKPUTMFXZF6ICYSJI6GNVI3UNTPONANCNFSM5ISQLGRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.