-
Notifications
You must be signed in to change notification settings - Fork 905
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
Adding support for Functions in crank validate (fixes #5491) #5570
Conversation
Signed-off-by: Jiri Tyr <jiri.tyr@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @jtyr, thanks for taking the initiative to fix this issue too! 🙇♂️
Some high level questions/feedback:
- Is it not necessary to also add Functions to the block in https://github.com/crossplane/crossplane/blob/master/cmd/crank/beta/validate/manager.go#L118-L134? when i did a quick look earlier, that's where I thought needed to be updated - is that for a different scenario maybe?
- Do you have some details you can add to the PR body for how you tested this change? e.g. what manifests/commands did you run and what was their output to show that the functionality is now correct?
Thanks for your feedback, @jbw976. I have updated the PR description with the way how this was tested. Not sure if the portion of the code you are referring to is relevant to this issue. I was just following the places where code is failing and fixed it so it doesn't fail anymore. But if you wish, I can add the missing portion of the code to this PR as it looks it's missing there anyway. I have tested it already and it doesn't seem to influence the functionality of this fix in any way. |
I don't think we need to update that portion of the code, we don't have XRDs in functions, and the only CRD should be the one for the input, which we don't need. |
@jbw976 If we don't have to add any other code then let's merge it so it at least works well for Configurations without version constraints. |
Successfully created backport PR for |
Description of your changes
This PR is fixing issue described in the issue #5491. Without this fix, the
crossplane beta validate
execution fails because there is no image detected. There is still an issue with version constraints (e.g.>=v1.2.3
) as the current implementation takes the version string as is and tries to fetch the image with that exact tag.Fixes #5491
I have:
make reviewable
to ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.y
labels to auto-backport this PR.The PR was tested like this:
Clone the repo, apply patch and build a new
crossplane
version:Create directory structure for the test:
Create files:
Run local Docker registry:
Package and push the
foo
package:Validate the
Bar
composition:crossplane beta render test/claim.yaml test/composition.yaml test/functions --include-full-xr | crossplane beta validate test/validation -
Output without the patch (missing image):
Output with the patch (image is found):
When I change the
apis/configuration.yaml
to use version constraints:The output even with the patch is this (image tag is the same as the version constraint and the validator fails to find such image):