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 update-rules command to update publishing-bot rules #253

Merged
merged 7 commits into from
Feb 11, 2022

Conversation

navidshaikh
Copy link
Member

@navidshaikh navidshaikh commented Apr 5, 2021

Description:

  • Add cmd/update-rules to update publishing-bot rules
  • Required flags: --rules, --branch
  • Exports the updated rules to file pointed by -o otherwise on prints stdout
  • Pin go version provided by --go flag else leave the field empty (omitempty)
  • Refer master branch rule per destination repo to add/update rule for provided branch

Usage:

Usage:  update-rules --branch BRANCH --rules PATH [--go VERSION | -o PATH]

Examples:

  • Update rules for branch release-1.21 with go version 1.16.4
  update-rules -branch release-1.21 -go 1.16.4 -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml
  • Update rules using URL to input rules file
update-rules -branch release-1.21 -go 1.16.4 -rules https://raw.githubusercontent.com/kubernetes/kubernetes/master/staging/publishing/rules.yaml
  • Update rules and export to /tmp/rules.yaml
  update-rules -branch release-1.22 -go 1.17.1 -o /tmp/rules.yaml -rules /go/src/k8s.io/kubernetes/staging/publishing/rules.yaml

fixes #252

TODO:

  • command
  • tests
  • docs

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 5, 2021
@k8s-ci-robot k8s-ci-robot requested review from mfojtik and nikhita April 5, 2021 05:17
Copy link
Member

@nikhita nikhita left a comment

Choose a reason for hiding this comment

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

@navidshaikh thanks for working on this, left some suggestions :)

glog.Fatalf("update failed, found invalid rules after update: %v", err)
}

data, err := yaml.Marshal(rules)
Copy link
Member

Choose a reason for hiding this comment

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

This removes comments...maybe consider using go-yaml/yaml to preserve comments (go-yaml/yaml#132)

Copy link
Member Author

@navidshaikh navidshaikh Apr 6, 2021

Choose a reason for hiding this comment

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

The comments are removed while the yaml content are deserialized into native ds. So at this point we don't have comments at all. To preserve comments, we'd need to load them into yaml.Node{} and navigate using it (we may also need to update the way rules loading and validations done in cmd/publishing-bot/config/rules.go). It's available in yaml.v3, currently we are using yaml.v2 here.

@navidshaikh
Copy link
Member Author

Updated PR to print the updated rules on stdout if no -o flag is specified, this allows providing input rules by URL (using --rules flag).

@puerco
Copy link
Member

puerco commented May 13, 2021

/cc

@k8s-ci-robot k8s-ci-robot requested a review from puerco May 13, 2021 17:51
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2021
@navidshaikh navidshaikh requested a review from nikhita May 31, 2021 16:00
@navidshaikh
Copy link
Member Author

/retitle Add update-rules command to update publishing-bot rules

@k8s-ci-robot k8s-ci-robot changed the title WIP: Add update-rules command to update publishing-bot rules Add update-rules command to update publishing-bot rules May 31, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2021
@navidshaikh
Copy link
Member Author

Rebased and squashed the commits.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2021
@navidshaikh
Copy link
Member Author

/cc @nikhita @cpanato

this is ready for review, PTAL at PR desc for usage/examples

@k8s-ci-robot k8s-ci-robot requested a review from cpanato June 15, 2021 18:02
@palnabarun
Copy link
Member

I had tried to generate the rules in kubernetes/kubernetes#103844 using the code here.

Some general observations:

  1. The comments in rules.yaml get removed after running this. I think this is a blocker to use this tool.
  2. The order of the keys in dictionaries gets changed. This is not a blocker as the key reordering happens only once. Post the initial change, the diff would be lower as the keys would be sorted in a previous rule update.
  3. We should have a Makefile with a recipe to install the updates-rules binary. This is also good to have as other parties can install the tool consistently.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 27, 2021
@xmudrii
Copy link
Member

xmudrii commented Nov 10, 2021

@navidshaikh @nikhita @palnabarun Is there any update on this PR? It would be nice to try to get this done -- it's definitely a very useful tool.

/remove-lifecycle stale

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 13, 2021

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2021
@navidshaikh
Copy link
Member Author

Is there any update on this PR? It would be nice to try to get this done -- it's definitely a very useful tool.

@xmudrii sorry for late reply! will work through the feedback now.

@palnabarun : thanks for the review!

The comments in rules.yaml get removed after running this. I think this is a blocker to use this tool.

This tool uses the existing utils (using yaml.v2) to load the rules. To retain the comments I think we'll need yaml.v3.

Question: Should we have alternate yaml loading utils (with yaml.v3) and use it with this tool to retain the comments?
or move the existing utils to yaml.v3?

We should have a Makefile with a recipe to install the updates-rules binary. This is also good to have as other parties can install the tool consistently.

👍

`make build` builds the CLI binary
`make build-image` includes the CLI binary in the image

Signed-off-by: Navid Shaikh <[email protected]>
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

Echoing from Slack:

@dims @sttts -- Do we feel this is in a place that we could merge/iterate?
I feel like we could have @kubernetes/release-engineering start kicking the tires and cleaning up as needed.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2021
@justaugustus
Copy link
Member

/assign @dims @sttts

@justaugustus
Copy link
Member

/assign @nikhita
ref: Slack

@palnabarun
Copy link
Member

Question: Should we have alternate yaml loading utils (with yaml.v3) and use it with this tool to retain the comments?
or move the existing utils to yaml.v3?

Parsing YAML comments is still non-trivial and I feel specifically for this feature, the benefits do not outweigh the effort needed.

Looking back at the usage of comments in the latest rules.yaml:

# TODO: remove bazel related files after we stop publishing branches with
# bazel files
# See: https://github.com/kubernetes/enhancements/issues/2420

The above blob of comments can be in a GH issue instead of residing in a comment.

With that, we may not need to parse comments at all and comment parsing becomes a non-blocker.

Thanks @navidshaikh for working on this! ❤️

/lgtm

@palnabarun
Copy link
Member

/hold for other reviews

@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 Dec 14, 2021
@navidshaikh
Copy link
Member Author

The above blob of comments can be in a GH issue instead of residing in a comment.

👍
There is only one comment in the rules file related to the KEP-2420: Reducing Kubernetes Build Maintenance. We can track updating the rules file part of the KEP tracking issue.

cc: @BenTheElder WDYT?

@justaugustus
Copy link
Member

@kubernetes/publishing-bot-maintainers -- Where did we land with this one?

@dims
Copy link
Member

dims commented Feb 11, 2022

/hold cancel
/approve

@palnabarun @justaugustus what are the follow ups after this merges?

@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 Feb 11, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, justaugustus, navidshaikh

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 29e3523 into kubernetes:master Feb 11, 2022
@navidshaikh navidshaikh deleted the pr/rules-robot branch February 17, 2022 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a tool to update publishing-bot rules for new releases
10 participants