lunny/helm-chart
1
0
Fork 0
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Fix unittests #434

Merged
pat-s merged 5 commits from refs/pull/434/head into main 2023-04-14 06:45:37 +00:00
Conversation 4 Commits 5 Files Changed 4 +14 -3
pat-s commented 2023-04-13 08:22:25 +00:00 (Migrated from gitea.com)
Copy Link

Unclear why it only appeared now and not earlier.

Unclear why it only appeared now and not earlier.
justusbunsi commented 2023-04-13 10:34:07 +00:00 (Migrated from gitea.com)
Copy Link

Based on the build logs the recent build downloaded https://github.com/helm-unittest/helm-unittest/releases/tag/v0.3.1. The last main build https://github.com/helm-unittest/helm-unittest/releases/tag/v0.3.0. From a quick glance I don't agree with the changes. We should not need to add any signing key related values if the signing is disabled.

Based on the build logs the recent build downloaded https://github.com/helm-unittest/helm-unittest/releases/tag/v0.3.1. The last main build https://github.com/helm-unittest/helm-unittest/releases/tag/v0.3.0. From a quick glance I don't agree with the changes. We should not need to add any signing key related values if the signing is disabled.
pat-s (Migrated from gitea.com) reviewed 2023-04-13 11:16:06 +00:00
unittests/statefulset/signing-disabled.yaml
@ -0,0 +17,4 @@
- it: skips gpg env in `init-directories` init container
template: templates/gitea/statefulset.yaml
set:
signing.enabled: false
pat-s (Migrated from gitea.com) commented 2023-04-13 11:16:06 +00:00
Copy Link

It looks like as if this should be signing.enabled: false and the respective secret should/could then be removed.

It looks like as if this should be `signing.enabled: false` and the respective secret should/could then be removed.
pat-s (Migrated from gitea.com) commented 2023-04-13 14:42:21 +00:00
Copy Link

You are right. Good catch.

You are right. Good catch.
pat-s commented 2023-04-13 11:17:03 +00:00 (Migrated from gitea.com)
Copy Link

We should not need to add any signing key related values if the signing is disabled.

The tests failed for the cases in which signing is enabled and hence were missing either a key or a secret. I think the new failure is valid and the non-failure previously might actually be a false-positive pass.

There is one case in the filed marked above where the signing.enabled value might be wrong, according to the unittest scope.

> We should not need to add any signing key related values if the signing is disabled. The tests failed for the cases in which signing is enabled and hence were missing either a key or a secret. I think the new failure is valid and the non-failure previously might actually be a false-positive pass. There is one case in the filed marked above where the `signing.enabled` value might be wrong, according to the unittest scope.
justusbunsi commented 2023-04-13 16:06:14 +00:00 (Migrated from gitea.com)
Copy Link

After having a more detailed look at the changes, I fully agree.

  1. The test in unittests/statefulset/signing-disabled.yaml is incorrect. The fact that it did not fail previously shows that this test case is currently not testing the correct thing - or does not assert all necessary parts.
    Other tests might be not fully reliable, too.
  2. https://github.com/helm-unittest/helm-unittest/pull/132 fixed several issues. One of the many changes there reveals wrong assumption in our test cases. I locally built the plugin and narrowed down the newly prompted error to this specific PR.

Additionally, we should pin the helm plugin version in our builds together with a --version ... in the install instructions inside CONTRIBUTING.md. That way we have much more stable build environment. And let renovate bump both references together.

After having a more detailed look at the changes, I fully agree. 1. The test in `unittests/statefulset/signing-disabled.yaml` is incorrect. The fact that it did not fail previously shows that this test case is currently not testing the correct thing - or does not assert all necessary parts.\ Other tests might be not fully reliable, too. 2. https://github.com/helm-unittest/helm-unittest/pull/132 fixed several issues. One of the many changes there reveals wrong assumption in our test cases. I locally built the plugin and narrowed down the newly prompted error to this specific PR. Additionally, we should pin the helm plugin version in our builds together with a `--version ...` in the install instructions inside `CONTRIBUTING.md`. That way we have much more stable build environment. And let renovate bump both references together.
justusbunsi (Migrated from gitea.com) reviewed 2023-04-13 18:19:14 +00:00
justusbunsi commented 2023-04-13 18:40:20 +00:00 (Migrated from gitea.com)
Copy Link
+      signing.privateKey: /raw/private.asc

This key usually holds the armored string of the private key, not the file reference. For testing purposes this would not make a difference but would be more consistent.

Otherwise LGTM

``` + signing.privateKey: /raw/private.asc ``` This key usually holds the armored string of the private key, not the file reference. For testing purposes this would not make a difference but would be more consistent. Otherwise LGTM
justusbunsi (Migrated from gitea.com) approved these changes 2023-04-14 06:44:58 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

LGTM

LGTM
Sign in to join this conversation.
Reviewers
No Reviewers
justusbunsi
Labels
Clear labels
has/backport
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/dependency
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
upstream/other
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#434
No description provided.
Delete Branch "refs/pull/434/head"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?