Conversation
Signed-off-by: Austin Abro <austinabro321@gmail.com>
✅ Deploy Preview for zarf-docs canceled.
|
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
soltysh
left a comment
There was a problem hiding this comment.
Two things:
- Can you add a unit-test to ensure we don't cause a regression.
- I'm not sure I'm recreating the problem correctly, I've tried this with
zarf package create examples/yolo/ -cthe resultingzarf.yamlstill points to the same files asBut we package both of these files asLines 14 to 15 in bb4083d
multi-games-0.yamlandmulti-games-1.yamlinside thecomponentsyolo-games.tarinzarf-package-yolo-amd64.tar.zst. So either I'm reproducing the problem the wrong way, or they fix is not correct.
|
The problem is in the check that we currently have inside of Zarf skips permission errors with Here are reproduction steps:
sudo tee /root/secret-ns.yaml >/dev/null <<'EOF'
apiVersion: v1
kind: Namespace
metadata:
name: demo
EOF
kind: ZarfPackageConfig
metadata:
name: repro
version: 1.0.0
components:
- name: ns
required: true
manifests:
- name: ns
namespace: demo
files:
- ../../../../../../../../../../root/secret-ns.yaml
|
We don't currently have any unit tests for deploy.go. Stems partially from the fact that it's hard to unit test our calls to Helm. I feel confident in our e2e tests here, and I see this more as a feature removal (backwards compatibility shim removal) so this won't be a case that comes back. Thoughts on tabling this until we get a better grasp on a testing strategy for deploy? |
soltysh
left a comment
There was a problem hiding this comment.
Confirmed, I was able to reproduce this w/o the fix and working with the fix. Although I will admit that the zarf.yaml contents inside the package, whether you manually extract it or use zarf package inspect definition could also be updated not to point to the source location, but rather package relative. But that's not blocking this PR.
lgtm
Description
#1469 changed how manifests were loaded into a package, but kept a shim around in case old packages were deployed on newer versions of Zarf. This shim can cause a bug under specific circumstances, see #4819.
Some consideration for adding a minimumVersionRequirement here, but given that this package layout change happened in v0.27.0 from May 2023, I deemed this not practically necessary.
The breaking change here is that we will no longer deploy packages from <v0.27.0. I doubt this is relevant to anyone.
Related Issue
Fixes #4819
Checklist before merging