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.

Sign up for GitHub

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

Jump to bottom

Add krel validate cmd #3765

Merged
merged 1 commit into from
Oct 4, 2024
Merged

Add krel validate cmd #3765

merged 1 commit into from
Oct 4, 2024

Conversation

npolshakova
Copy link
Contributor

@npolshakova npolshakova commented Sep 17, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Introduces new krel validate <release-notes-maps> command for release notes team to use when checking for invalid formatting.

The release notes team is blocked whenever invalid yaml is merged into sig-release:

kubernetes/sig-release#2589
kubernetes/sig-release#2541

Invalid yaml can be introduced during the review process when people apply changes or manually edit the maps (not through krel, but in an editor).

Examples of runs:

Valid check:

krel validate sig-release/releases/release-1.30/release-notes/maps
...
Validating YAML file: sig-release/releases/release-1.30/release-notes/maps/pr-123935-map.yaml
File sig-release/releases/release-1.30/release-notes/maps/pr-123935-map.yaml is valid YAML.
YAML file sig-release/releases/release-1.30/release-notes/maps/pr-123935-map.yaml is valid.
Validating YAML file: sig-release/releases/release-1.30/release-notes/maps/pr-124001-map.yaml
File sig-release/releases/release-1.30/release-notes/maps/pr-124001-map.yaml is valid YAML.
YAML file sig-release/releases/release-1.30/release-notes/maps/pr-124001-map.yaml is valid.
All release notes are valid.

Invalid yaml:

Validating YAML file: sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml
FATA failed to validate release notes: YAML validation failed for sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml: YAML unmarshal failed for sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml:error converting YAML to JSON: yaml: line 3: found character that cannot start any token

Which issue(s) this PR fixes:

Catches occurrences of #2753

Special notes for your reviewer:

Eventually, this command can be added to release-actions and run as a workflow similar to kubernetes/sig-release#2547.

Does this PR introduce a user-facing change?

Introduced new `krel validate` command for validating committed release note edits.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Sep 17, 2024
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2024
@npolshakova
Copy link
Contributor Author

@satyampsoni this can be used to validate the yamls instead of relying on the script. We can move this to a GitHub action once it goes in.

@npolshakova npolshakova force-pushed the npolshak/add-krel-validation-cmd branch from 9e25fc3 to 200851d Compare September 17, 2024 19:49
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2024
@npolshakova npolshakova force-pushed the npolshak/add-krel-validation-cmd branch from 200851d to 92cdea1 Compare September 17, 2024 19:50
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 17, 2024
@npolshakova npolshakova force-pushed the npolshak/add-krel-validation-cmd branch 2 times, most recently from 2f93528 to 21c7f05 Compare September 17, 2024 20:11
xmudrii
xmudrii requested changes Sep 18, 2024
View reviewed changes
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate_test.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
cmd/krel/cmd/validate.go Show resolved Hide resolved
@npolshakova npolshakova requested a review from xmudrii September 18, 2024 20:32
@npolshakova npolshakova force-pushed the npolshak/add-krel-validation-cmd branch from 4e62186 to 28b4db2 Compare September 30, 2024 15:49
@npolshakova
Copy link
Contributor Author

@kubernetes/sig-release-leads @xmudrii Can you give this another pass when you have a chance? Thanks!

cpanato
cpanato approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

but i will elave for @xmudrii for final review

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2024
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 2, 2024
xmudrii
xmudrii requested changes Oct 2, 2024
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

Just a few questions and nits, otherwise LGTM :)

Sorry for the delay!

cmd/krel/cmd/testdata/validation-data/invalid-indent.yaml Outdated Show resolved Hide resolved
cmd/krel/cmd/testdata/validation-data/invalid-multi-line.yaml Outdated Show resolved Hide resolved
cmd/krel/cmd/testdata/validation-data/invalid-yaml-start.yaml Outdated Show resolved Hide resolved
cmd/krel/cmd/testdata/validation-data/invalid-indent.yaml Show resolved Hide resolved
cmd/krel/cmd/validate.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2024
@npolshakova npolshakova force-pushed the npolshak/add-krel-validation-cmd branch from 28b4db2 to 0d66a2a Compare October 2, 2024 15:57
@npolshakova
Copy link
Contributor Author

npolshakova commented Oct 2, 2024 •
edited
Loading

Just a few questions and nits, otherwise LGTM :)

Sorry for the delay!

Thanks @xmudrii for the review! Here's the updated behavior with the cmd flag:

If it's not provided:

❯ go run cmd/krel/main.go release-notes validate 
FATA path to release notes must be provided via --path-to-release-notes 
exit status 1

If it's provided:

❯ go run cmd/krel/main.go release-notes validate --path-to-release-notes ~/kubernetes/sig-release/releases/release-1.30/release-notes
Validating YAML file: /Users/ninapolshakova/kubernetes/sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml
FATA validating release notes: validating YAML file /Users/ninapolshakova/kubernetes/sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml: punctuation check for file /Users/ninapolshakova/kubernetes/sig-release/releases/release-1.30/release-notes/maps/pr-112957-map.yaml: the 'text' field does not end with valid punctuation: '"kubelet allowed specifying a custom root directory for pod logs (instead of the default /var/log/pods) using the `podLogsDir` key in kubelet configuration."
' 
exit status 1

Then the top level krel help message:

❯ go run cmd/krel/main.go 
Usage:
  krel release-notes [flags]
  krel release-notes [command]

Available Commands:
  validate    The subcommand for validating release notes for the Release Notes subteam of SIG Release

@npolshakova npolshakova requested a review from xmudrii October 2, 2024 15:59
xmudrii
xmudrii approved these changes Oct 4, 2024
View reviewed changes
Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, npolshakova, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xmudrii
Copy link
Member

xmudrii commented Oct 4, 2024

:shipit:
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit 613836f into kubernetes:master Oct 4, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xmudrii xmudrii xmudrii approved these changes

@cpanato cpanato cpanato approved these changes

@jimangel jimangel Awaiting requested review from jimangel

@salaxander salaxander Awaiting requested review from salaxander

Assignees

@cpanato cpanato

@xmudrii xmudrii

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@npolshakova @k8s-ci-robot @xmudrii @cpanato