Improve autobump documentation #20458
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @mpherman2. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
k8s-ci-robot
added
the
sig/testing
|
/ok-to-test This looks good to me. /lgtm /hold |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
lgtm
| StagingRefConfigFile string `yaml:"stagingRefConfigFile"` | ||
| //Repo used when generating pull request | ||
| // Repo used when generating pull request |
There was a problem hiding this comment.
Not sure I understand exactly what is meant by source or target repo, but I updated the comment to more explicitly define what repo this field refers to and how it get used.
|
/test pull-test-infra-integration |
mpherman2
force-pushed
the
documentation_cleanup
branch
from
053ba0b to
162f61d
Compare
k8s-ci-robot
removed
the
lgtm
| StagingRefConfigFile string `yaml:"stagingRefConfigFile"` | ||
| //Repo used when generating pull request | ||
| // Repo that the PR for the bump will be pushed to. This is used to generate comparison link in the PR summary. |
There was a problem hiding this comment.
What? Those are two different repos for everyone else: One repo the PR gets created against, one for the diffing
There was a problem hiding this comment.
Yeah, I tried to clarify the distinction between the different repos at play in my review comment. I think we're talking about at least 3 repos in total if we include the Bot's fork. We could even be talking about 4 if the reference file used to choose the next version to bump to is in another repo.
There was a problem hiding this comment.
Sorry about the misunderstanding. I was confused as to exactly how this was used and I was looking at an example where the source and target repo were the same. This refers to the source repo
| StagingRefConfigFile string `yaml:"stagingRefConfigFile"` | ||
| //Repo used when generating pull request | ||
| // Repo that the PR for the bump will be pushed to. This is used to generate comparison link in the PR summary. |
There was a problem hiding this comment.
"Repo that the PR for the bump will be pushed to." is a bit confusing to me since we push the PR branch to the fork of the target repo, but the PR is created in the target repo itself.
Furthermore, the comparison link that is used in the PR summary should actually come from the upstream repo where the images are sourced in order to actually show the changes made between the old version and the version to which we are bumping.
So if we're bumping Prow images in GoogleCloudPlatform/oss-test-infra (the target repo), the fork would be something like bot-name/oss-test-infra and the source repo of the images to use for comparison links would be kubernetes/test-infra. Which of these repos is this field intended to represent? Do we need more fields to distinguish these?
There was a problem hiding this comment.
Oh! This makes much more sense now. I was only looking at examples where the target and source repo were the same and so I was confused as to what the distinction was. This is meant to refer to the source repo. Thank you for the explanation
experiment/autobumper/README.md
Outdated
| * summarise: Whether or not the format of the PR summary for this prefix should be summarised. | ||
| * consistentImages: Whether the prefix tags should be consistent after the bump | ||
|
|
||
| * For info about what should go in the config look at [the documentation for the Options here](https://github.com/kubernetes/test-infra/blob/0980814b76d8/experiment/autobumper/bumper/bumper.go#L86) and look at the example below. |
There was a problem hiding this comment.
Please link to the official GoDoc site instead: https://pkg.go.dev/k8s.io/test-infra/experiment/autobumper/bumper#Options
(Navigates more nicely and doesn't have to be pinned to a specific commit so it will automatically stay up to date.)
mpherman2
force-pushed
the
documentation_cleanup
branch
2 times, most recently
from
8257888 to
882fe0e
Compare
mpherman2
force-pushed
the
documentation_cleanup
branch
from
882fe0e to
f7466ec
Compare
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, chaodaiG, cjwagner, mpherman2 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 |
Following up on comments from old PRs. Linked to godocs in the README and improved the comments for options to be more clear about what each field does and when it is required. If any of these fields are still ambiguous and need more clarification please let me know.