Empty manifests should result in empty output

[kavfixnel]

Description

Removes the if statement to check if manifests are empty and updates the test accordingly

Fixes:
#363

Checklist

Please make sure that your PR fulfills the following requirements:

• Reviewed the guidelines for contributing to this repository
• The commit message follows the Conventional Commits Guidelines.
• Tests for the changes have been updated
• Are you adding dependencies? If so, please run go mod tidy -compat=1.17 to ensure only the minimum is pulled in.
• Docs have been added / updated
• Optional. My organization is added to USERS.md.

Type of Change

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• New tests
• Build/CI related changes
• Documentation content changes
• Other (please describe)

Other information

[CLAassistant]

All committers have signed the CLA.

[codecov-commenter]

Codecov Report

Merging #364 (df5b687) into main (9c7288a) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #364      +/-   ##
==========================================
- Coverage   77.64%   77.60%   -0.05%     
==========================================
  Files          22       22              
  Lines        1011     1009       -2     
==========================================
- Hits          785      783       -2     
  Misses        142      142              
  Partials       84       84              

Impacted Files	Coverage Δ
cmd/generate.go	73.33% <ø> (-0.87%)	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[werne2j]

@AlexanderProschek Seems like a valid use case. Thanks for the contribution!

[werne2j]

Looks good! Will wait for @jkayani to take a look before merging.

[jkayani]

The "No manifests" error was introduced here: #231. If we take this patch, doesn't that mean we'll have this issue again with the AVP integration with other tools, right?

What about a switch --allow-empty to allow this behavior but make it opt-int?

[werne2j]

Hmm.. i forgot about that issue. I would like to think that we could handle both scenarios without a flag. 🤔

[kavfixnel]

@jkayani if I understand correctly, what your change in #231 adds is the ability for AVP to act as a sort of guard against other programs mistakes and halt execution. By that same reasoning this should also fail:

~ ❯ ((>&2 echo "some error" && exit 1) | kubectl diff -f -); echo $?;
some error
0

which it does not. kubectl will happily take an empty yaml node.

My point (see #363) is that unlike AVP, other Kubernetes tools like kubectl and ArgoCD don't fail executing on an empty input, rather do exactly what the input tells them to do (empty in -> empty out). Convention requires errors to be handled before inputting things into a program.

May I suggest adding a debug flag that add some useful debugging info, including if the input is empty?

[jkayani]

Re-reading your issue and re-visiting the problem from the previous PR, I think you make a good case. At that time I'd also forgotten about set -o pipefail which would've solved the problem without any AVP code changes - I ran this test to confirm using the release of AVP before that previous PR was merged:

$ sh -c '((>&2 echo "some error" && exit 1) | avp_old generate -c ~/Documents/ops/argo/vault/ibm-sm-creds.yaml -); echo $?;'
some error
0

$ sh -c 'set -o pipefail; ((>&2 echo "some error" && exit 1) | avp_old generate -c ~/Documents/ops/argo/vault/ibm-sm-creds.yaml -); echo $?;'
some error
1

$ avp_old version
argocd-vault-plugin v1.5.0 (89e02217591d856e3902be247df36ec7d489270f) BuildDate: 2021-11-01T15:53:36Z

So given that kubectl and Argo CD are OK with empty manifests, you're right, I think we should be too. It'd also be good to remind users (like past me) about -o pipefail to help them debug situations via the docs. We do have the verbose mode introduced in #350 but it's no use unless it's explicitly enabled. If you wouldn't mind adding to the docs as part of this PR, I'm happy to accept this.

[kavfixnel]

@jkayani I added some docs in the howitworks README. Let me know if this is good or if you were looking for something else