test-infra
test-infra copied to clipboard
kubernetes
Retain LGTM through squashes for kubernetes/kubernetes repo
Welcome @gdsoumya!
It looks like this is your first PR to kubernetes/test-infra 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes/test-infra has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @gdsoumya. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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.
Ironically, my only nit would be to squash the two commits :)
/approve /hold for nit
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: BenTheElder, chases2, gdsoumya
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~config/OWNERS~~ [BenTheElder,chases2]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
This was originally enabled in test-infra but disabled in https://github.com/kubernetes/test-infra/pull/14426. The only reason mentioned is this :grimacing:
Also disable the tree hash thing that doesn't seem to be useful
@BenTheElder since you had lgtm'd that PR, do you recall specific reasons why this isn't being used in test-infra today?
I would just feel more confident rolling this out in k/k if k/test-infra endorsed it :smile:
For what it's worth, this config option will only retain lgtm when the existing commits in a PR are squashed but NOT when the PR is rebased because that will change the tree hash. This seems like a safe option so I'm not really opposed to enabling this but I'm still interested to know why test-infra disabled it.
Yep, if your rebase does anything except a squash against the same merge base then an existing LGTM still gets cancelled.
Memory is hazy but I recall it wasn't working (in a harmless lgtm is still lost on squash kind of way) at one point and hadn't seen it work in test-infra. sftim reports it working in website, I'm happy to enable it in test-infra and kind first and do more testing if we need but I also trust sftim.
Perhaps we can reference an example of WAI?
That's not quite the same thing, but I added an empty commit and then squashed it out with no trouble.
Lgtm from me, but would also ack from @cblecker.
/assign @cblecker
We'll also need to send a notice to k-dev once this is rolled out.
I have a feeling that the initial implementation didn't work right, but it's been behaving as advertised in k/website for some time.
/test pull-test-infra-verify-lint /lgtm
So I did a bunch of testing on this, and the plugin does work as expected. I tried to get around it in a bunch of ways and was unable to. I think this is relatively safe.
I know it definitely wasn't working as expected back in 2018/2019 when @matthyx originally wrote the code.. I don't know if @nikhita's cleanup (https://github.com/kubernetes/test-infra/pull/10579) helped it, although I can't see exactly why. The only other thing I can think of is GitHub fixed some sort of bug in the tree hash they were returning.
We still need a roll out plan with communications, but I have no technical objections to moving this forward.
If we can confirm it's now a solid foundation, I'll check whether we can skip CI using the same mechanism... Thanks for following up on this @cblecker
Anyone want to blog about this on https://k8s.dev/? (if so, there are folks who can help make that happen)
I am +1 in concept. I don't have context on it working or not currently or what process / approvals we need. cc @mrbobbytables
I am +1 on the idea as well. We should get comms rolling and maybe time it after 1.24 code thaw? Thoughts?
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: gdsoumya / name: Soumya Ghosh Dastidar (f8b688eea890edb0c25b814bbdab992ddbafc5d3, f1a6ce56d5da44c7b630ceba4ea4c04f1bca3fc0)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}