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

Add unknown field check to beta validate #5791

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

enesonus
Copy link
Contributor

@enesonus enesonus commented Jun 19, 2024

Description of your changes

This PR implements a validation logic that use pruning to get the unknown fields of a resource. With the help of this new validation step we can get informed about the typos and indentation errors.

Here are some examples of this new validation:

  1. XR validation:

xrd.yaml
xr.yaml:

apiVersion: example.crossplane.io/v1beta1
kind: UserAccess
metadata:
  name: example
spec:
  count: 2
  accesKey: true  # an unknown field

Output:

go run cmd/crank/main.go beta validate ./.work/xfn/ ./.work/xfn/xr.yaml
[x] schema validation error example.crossplane.io/v1beta1, Kind=UserAccess, example : spec.accesKey: Invalid value: "accesKey": unknown field: "accesKey"
Total 1 resources: 0 missing schemas, 0 success cases, 1 failure cases
crossplane: error: cannot validate resources: could not validate all resources
exit status 1
  1. Piping crossplane beta render function with field errors at composition:

Uses jbw976/demo-upbound@38017fa which has a typo, indentation error and unknown field

crossplane beta render .work/xfn/xr.yaml  .work/xfn/composition.yaml  .work/xfn/functions.yaml --include-full-xr | go run cmd/crank/main.go beta validate .work/xfn/ - 
package schemas does not exist, downloading:  xpkg.upbound.io/upbound/provider-aws-iam:v1.6.0
[x] schema validation error example.crossplane.io/v1beta1, Kind=UserAccess, example : spec.accesKey: Invalid value: "accesKey": unknown field: "accesKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.forProvider.userSelectorFoo: Invalid value: "userSelectorFoo": unknown field: "userSelectorFoo"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.pgpKey: Invalid value: "pgpKey": unknown field: "pgpKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-0 : spec.writeConnectionSecretToRef.fookey: Invalid value: "fookey": unknown field: "fookey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.forProvider.userSelectorFoo: Invalid value: "userSelectorFoo": unknown field: "userSelectorFoo"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.pgpKey: Invalid value: "pgpKey": unknown field: "pgpKey"
[x] schema validation error iam.aws.upbound.io/v1beta1, Kind=AccessKey, sample-access-key-1 : spec.writeConnectionSecretToRef.fookey: Invalid value: "fookey": unknown field: "fookey"
[✓] iam.aws.upbound.io/v1beta1, Kind=User, test-user-0 validated successfully
[✓] iam.aws.upbound.io/v1beta1, Kind=User, test-user-1 validated successfully
Total 5 resources: 0 missing schemas, 2 success cases, 3 failure cases
crossplane: error: cannot validate resources: could not validate all resources
exit status 1

Fixes: #5376

I added the relevant changes made in this PR to docs: crossplane/docs#790

I have:

  • Read and followed Crossplane's contribution process.
  • Run earthly +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.
  • Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

Super cool @enesonus to see the validate functionality getting extended with more useful checks - great start to your LFX mentorship summer 💪

All the validation looks to be working correctly, I've added just a few comments for you to consider 🙇‍♂️

cmd/crank/beta/validate/unknown_fields.go Outdated Show resolved Hide resolved
cmd/crank/beta/validate/unknown_fields.go Outdated Show resolved Hide resolved
cmd/crank/beta/validate/unknown_fields.go Show resolved Hide resolved
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

OK @enesonus, I responded to the comments here, so please go ahead and update the code to use the invalid error type, and then add some unit tests (e.g. new test cases in validate_test.go) 🙇‍♂️

@enesonus enesonus mentioned this pull request Jun 28, 2024
6 tasks
@enesonus enesonus force-pushed the add-more-validations branch 2 times, most recently from 1d70da9 to 6170555 Compare June 29, 2024 11:27
Signed-off-by: Mehmet Enes <[email protected]>
@enesonus enesonus marked this pull request as ready for review June 29, 2024 11:47
@enesonus enesonus requested review from phisco and a team as code owners June 29, 2024 11:47
@enesonus enesonus requested a review from turkenh June 29, 2024 11:47
Signed-off-by: Mehmet Enes <[email protected]>
@ezgidemirel
Copy link
Member

@enesonus thanks for the PR! This is the most requested/asked functionality for the validate command. Do you mind updating the example in the PR description with your latest changes (e.g., Internal error vs Invalid)?

@enesonus
Copy link
Contributor Author

enesonus commented Jul 1, 2024

@enesonus thanks for the PR! This is the most requested/asked functionality for the validate command. Do you mind updating the example in the PR description with your latest changes (e.g., Internal error vs Invalid)?

Thanks for the reminder! I updated it to the latest error string now.

Signed-off-by: Mehmet Enes <[email protected]>
Copy link
Member

@jbw976 jbw976 left a comment

Choose a reason for hiding this comment

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

This functionality looks great now, let's get it merged in so it can ship in v1.17! 🎉

@jbw976 jbw976 merged commit 5d53c2a into crossplane:master Jul 3, 2024
17 checks passed
@enesonus enesonus deleted the add-more-validations branch July 10, 2024 17:32
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.

Add more validation checks in validate command
3 participants