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

fix(k8s): regression in globs in k8s manifest files #4903

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

vvagaytsev
Copy link
Collaborator

What this PR does / why we need it:
The regression was introduced in #4516. After that PR, it was possible to silently skip all non-existing files specified in in spec.files of a Deploy action of type kubernetes.

In 0.13.6 that would fail with error like this:

ENOENT: no such file or directory, open '~/garden/core/test/data/test-projects/kubernetes-type/with-build-action/deployment-action1.yaml'

This PR implements some explicit checks for path existence and makes the output a bit more readable:

Invalid manifest file path(s) in Deploy action 'with-build-action'. Cannot find manifest file(s) at ~/garden/core/test/data/test-projects/kubernetes-type/.garden/build/exec-build/deployment-action1.yaml

Issues #4505 and #4506 still exist. That's why the deployment can fail even with the existing manifest file. The build directory appears to be empty. It will be fixed in a separate PR.

Which issue(s) this PR fixes:

Fixes #4900

Special notes for your reviewer:
You can find more detail and explanation in the commit messages.

The regression was introduced in #4516.
Initial implementation of glob patterns in files paths
could result into empty list of files.
This fixes the behaviour by fail-fast existence checks for manifest files.
Test coverage for `spec.files` of `Deploy` action of `kubernetes` type.

* Add own Deployment manifest file, because builds are not executed in the test.
* Add unit tests to document and cover the expected behaviour.

Initial changes from c9efb47 did not have effect.
The Deploy action `with-build-action` has never been called with `readManifests`.
@vvagaytsev vvagaytsev requested a review from a team July 31, 2023 08:54
thsig
thsig previously requested changes Jul 31, 2023
Copy link
Collaborator

@thsig thsig left a comment

Choose a reason for hiding this comment

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

Great stuff! Really appreciate the added test cases.

I had one suggestion to improve the new error message, otherwise we're good to go here.

@vvagaytsev vvagaytsev requested a review from thsig July 31, 2023 11:11
@vvagaytsev vvagaytsev force-pushed the fix/regression-globs-in-k8s-manifest-files branch from dad4395 to 18745e7 Compare July 31, 2023 11:19
Copy link
Contributor

@Walther Walther left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fix! ✨

@vvagaytsev vvagaytsev dismissed thsig’s stale review August 1, 2023 11:22

The changes have been addressed.

@vvagaytsev vvagaytsev merged commit 1b511dc into main Aug 1, 2023
@vvagaytsev vvagaytsev deleted the fix/regression-globs-in-k8s-manifest-files branch August 1, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Print a warning or something if a kubernetes deploy action specifies files which are not found
3 participants