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

Revert "workflows/eval: Add the eval summary as a comment" #361388

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Dec 3, 2024

This reverts commit 38003ce.
I am already overwhelmed with github notifications and this makes the situation even worse.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: policy discussion 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Dec 3, 2024
@Aleksanaa
Copy link
Member

Ideally it should only output one comment and then modify it continuously. The position it appears should be relatively high so will not be folded by GitHub.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Eval summary

  • Added packages: 0
  • Removed packages: 0
  • Changed packages: 0
  • Rebuild Linux: 0
  • Rebuild Darwin: 0

@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2024

Ideally it should only output one comment and then modify it continuously. The position it appears should be relatively high so will not be folded by GitHub.

It only updates the last comment already. Still when I open a pull request, I will now always get a notification even if no human has responded to my PR.

@wolfgangwalther
Copy link
Contributor

Ideally it should only output one comment and then modify it continuously.

That's already the case, IIUC.

The position it appears should be relatively high so will not be folded by GitHub.

Yeah, I suggested to put it before requesting reviews via OWNERS in #361061 (comment).

@wolfgangwalther
Copy link
Contributor

I will now always get a notification even if no human has responded to my PR.

Yeah. Annoying.

Is there any way to have the bot post the eval results into the PR description / body, maybe replacing a special placeholder in the template?

@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2024

I will now always get a notification even if no human has responded to my PR.

Yeah. Annoying.

Is there any way to have the bot post the eval results into the PR description / body, maybe replacing a special placeholder in the template?

Don't think github actions have currently permissions to do that.

@azuwis
Copy link
Contributor

azuwis commented Dec 3, 2024

I will now always get a notification even if no human has responded to my PR.

Yeah. Annoying.
Is there any way to have the bot post the eval results into the PR description / body, maybe replacing a special placeholder in the template?

Don't think github actions have currently permissions to do that.

gh pr edit <NUMBER> --body "BODY" should work?

@Mic92 Mic92 merged commit a59bc27 into NixOS:master Dec 3, 2024
16 of 17 checks passed
@Mic92
Copy link
Member Author

Mic92 commented Dec 3, 2024

I will now always get a notification even if no human has responded to my PR.

Yeah. Annoying.
Is there any way to have the bot post the eval results into the PR description / body, maybe replacing a special placeholder in the template?

Don't think github actions have currently permissions to do that.

gh pr edit <NUMBER> --body "BODY" should work?

Ok. Maybe you can try this in a follow up pull request.

@Mic92 Mic92 deleted the revert-ci-comments branch December 3, 2024 09:19
@azuwis
Copy link
Contributor

azuwis commented Dec 3, 2024

I will now always get a notification even if no human has responded to my PR.

Yeah. Annoying.

Is there any way to have the bot post the eval results into the PR description / body, maybe replacing a special placeholder in the template?

It's possible eff1eee

Demo azuwis#10

Screenshot 2024-12-03 at 18 29 13

@wolfgangwalther
Copy link
Contributor

Are you still receiving notifications about those edits or do those happen "silently"?

@azuwis
Copy link
Contributor

azuwis commented Dec 3, 2024

Are you still receiving notifications about those edits or do those happen "silently"?

No notifications.

For reference https://stackoverflow.com/questions/38986589/does-github-send-a-notification-when-an-issue-comment-is-edited

No, you do not get a notification if a comment, issue title, or description is edited or even deleted.

@mention should also get no notifications for edits, according to this:

Per my experiments, if edit a comment to add a @mention, the mentioned user will receive the notification, but only once, regardless of how many edits are done afterward and whether they are @mentioned or not.

@infinisil
Copy link
Member

Perhaps we can create a commit status instead, I think that's also what ofborg does: https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status

@wolfgangwalther
Copy link
Contributor

Perhaps we can create a commit status instead

I think the idea here is to make the number of rebuilt packages easier to see. The data is already available in the summary page for the eval jobs, so arguably a commit status... would not be any more visible than that?

@azuwis
Copy link
Contributor

azuwis commented Dec 4, 2024

I've made a version that add the eval summary to the Things done lists of the PR template master...azuwis:nixpkgs:push-rnzsmkkzkums, example:

  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.
  • Eval summary
    • package: added 0, removed 0, changed 61
    • rebuild: linux 0, darwin 61

Demo azuwis#15

I think it's pretty elegant, no notifications, occupy only a little space.

Before opening a PR, I'd like to know what you think.

@infinisil
Copy link
Member

@azuwis I'm not a huge fan, because at least in a lot of my PRs I completely remove the checkmarks and rewrite appropriate ones myself, and I don't really want CI to mess with that, especially when rebuild count is irrelevant anyways. Also it just seems conceptually wrong, the comment is user-written and not meant for CI output. I'm also concerned on what happens when a user updates the description around the same time as CI, that seems reconcilable, but that shouldn't even be a concern imo.

The data is already available in the summary page for the eval jobs, so arguably a commit status... would not be any more visible than that?

It takes two button presses to get to the summary page (the first one gets you to the details page, and then finding where the summary is is not obvious), while with a commit status it would be a single click that can lead to any page. And people are already used to that from ofborg.

@azuwis
Copy link
Contributor

azuwis commented Dec 5, 2024

Perhaps we can create a commit status instead, I think that's also what ofborg does: https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status

Good idea, implemented in #361973

Screenshot 2024-12-05 at 14 30 02

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants