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

Support clear bot history on new build #390

Merged
merged 5 commits into from
Sep 29, 2021

Conversation

fredagsfys
Copy link
Contributor

As per this issue - impl. support to clear bot history on new builds to prevent PR comment bloat.

@fredagsfys
Copy link
Contributor Author

fredagsfys commented Sep 3, 2021

I saw your discussion regarding hiding old bot comments, so you really need to preserve build comments? If not - this would support deleting them.

If this doesn't suffice I'll refactor this PR to post a graphql mutation to minimize comments. Let me now...

@markmandel
Copy link
Member

I didn't think to just delete the old comments on each run (rather than minimise), but it's not a bad idea to at least test.

I do like being able to see the history over time (so prefer minimisation), it can also make it easier to identify flaky tests, and I have some vague concerns about race conditions when builds are simultaneous, but I don't see any reason not to at least test it out.

I wonder if we'll hit API request limits in the beginning 😄

@XAMPPRocky did you have any thoughts? It was you who brought up removing/hiding the old test results, so wanted to check in first before deploying anything new.

@XAMPPRocky
Copy link
Collaborator

It seems fine to me. My one concern about deleting, is that it would make it harder to look at old builds, which sometimes you want to do, but I'm not against deleting either.

@markmandel
Copy link
Member

It seems fine to me. My one concern about deleting, is that it would make it harder to look at old builds, which sometimes you want to do, but I'm not against deleting either.

Yeah, same here. But let's try it! If we hate it, we can change it again. I'll get it built and deployed, and will confirm it works on this PR, and assuming it all works, we can roll it out and see how we go!

@markmandel
Copy link
Member

Haven't forgotten at this, just been at 100% bandwidth. Do want to get this live so we can take it for a spin, but would like to do it at a time so I can watch in case it goes badly for any reason 😄

@markmandel
Copy link
Member

Deploying now! Let's see how we go!

@markmandel
Copy link
Member

Looks like it worked! Let's let it bake for a few days (also would love to get that typo fixed), and if it's going swimmingly, let's merge 👍🏻

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 40fcfcc0-3b81-4fda-a10c-228ddc427fcb

To build this version:

git fetch git@github.com:googleforgames/quilkin.git pull/390/head:pr_390 && git checkout pr_390
cargo build

@markmandel
Copy link
Member

Let's merge it - it's looking good 👍🏻

@markmandel markmandel merged commit fb04da1 into googleforgames:main Sep 29, 2021
@markmandel markmandel added area/build-tools Development tooling. kind/feature New feature or request labels Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-tools Development tooling. cla: yes kind/feature New feature or request size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants