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 validate command #790

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

enesonus
Copy link
Contributor

Adds the improvements made at PR crossplane/crossplane#5791 to beta validate command's reference docs for informational purposes.

Copy link

netlify bot commented Jun 30, 2024

Deploy Preview for crossplane ready!

Name Link
🔨 Latest commit 0e2fd9e
🔍 Latest deploy log https://app.netlify.com/sites/crossplane/deploys/6685a78418d6e60008282b22
😎 Deploy Preview https://deploy-preview-790--crossplane.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 84 (🔴 down 1 from production)
Accessibility: 90 (🔴 down 2 from production)
Best Practices: 83 (no change from production)
SEO: 93 (no change from production)
PWA: 70 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Thanks for taking the initiative to make a docs update too @enesonus! a couple of suggestions for you to consider 🙇‍♂️

content/master/cli/command-reference.md Outdated Show resolved Hide resolved
content/v1.16/cli/command-reference.md Outdated Show resolved Hide resolved
@enesonus
Copy link
Contributor Author

enesonus commented Jul 3, 2024

Thanks for taking the initiative to make a docs update too @enesonus! a couple of suggestions for you to consider 🙇‍♂️

Thanks for the suggestions @jbw976, I updated the changes accordingly.

@@ -828,7 +828,9 @@ Configuration/platform-ref-aws v0.9.0 True

The `crossplane beta validate` command validates
[compositions]({{<ref "../concepts/compositions">}}) against provider or XRD
schemas using the Kubernetes API server's validation library.
schemas using the Kubernetes API server's validation library
with additional validation such as checking for unknown fields,
Copy link
Member

Choose a reason for hiding this comment

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

ha, looks like my suggestion introduced a Vale error in https://github.com/crossplane/docs/actions/runs/9758986765/job/26934740522?pr=790.

content/master/cli/command-reference.md
 832:[6](https://github.com/crossplane/docs/actions/runs/9758986765/job/26934740522?pr=790#step:5:7)  warning  'additional' is too wordy.  write-good.TooWordy

Vale is intended to help us write in a high quality consistent format, but its rules can be difficult to comply with sometimes 🤓

I think this is the rule that is failing:
https://github.com/crossplane/docs/blob/master/utils/vale/styles/write-good/TooWordy.yml#L17

additional is also mentioned in https://github.com/crossplane/docs/blob/master/utils/vale/styles/Microsoft/ComplexWords.yml#L21, which suggests using more or extra instead. So perhaps to make Vale happy we could just use extra instead of additional here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was actually aware of the warning, thought it is Vale's own warning and couldn't find the location to look for alternatives. Thanks for pointing out the guides @jbw976. I changed additional to extra. One question is should I leave it as with extra validation or change it to with extra validations what do you think? I think the latter one sounds better but we dont have more than one extra validation currently so I couldnt decide 😅

Copy link
Member

Choose a reason for hiding this comment

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

with extra validation sounds good enough to my ears! 👂 ✅

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.

awesome, this looks great to me now! Thanks @enesonus!!

@jbw976 jbw976 merged commit d741cb3 into crossplane:master Jul 5, 2024
7 checks passed
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.

2 participants