Skip to content

toolhive: init at 0.0.47 #420384

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

toolhive: init at 0.0.47 #420384

wants to merge 2 commits into from

Conversation

thrix
Copy link

@thrix thrix commented Jun 27, 2025 •
edited
Loading

Signed-off-by: Miroslav Vadkerti mvadkert@redhat.com

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/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (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, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

Add a 👍 reaction to pull requests you find important.

@thrix thrix marked this pull request as draft June 27, 2025 00:40
@nixpkgs-ci nixpkgs-ci bot added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 27, 2025
acid-bong
acid-bong suggested changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@acid-bong acid-bong left a comment

Choose a reason for hiding this comment

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

Moin and welcome. A couple of notes

acid-bong
acid-bong suggested changes Jun 27, 2025
View reviewed changes
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jul 1, 2025
@thrix thrix force-pushed the init-toolhive branch 4 times, most recently from 3f5098e to 61c1629 Compare July 3, 2025 02:02
@thrix thrix requested a review from acid-bong July 3, 2025 02:02
@thrix thrix marked this pull request as ready for review July 3, 2025 02:02
@thrix
Copy link
Author

thrix commented Jul 3, 2025

@acid-bong I believe I finally resolved all the comments, please re-review!

acid-bong
acid-bong suggested changes Jul 3, 2025
View reviewed changes
Copy link
Contributor

@acid-bong acid-bong left a comment

Choose a reason for hiding this comment

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

You only restored the maintainer entry, but did nothing to the package recipe.

Also an obligatory one: squash your edits, there should be one commit per addition or change (including maintainers), new packages/modules are created in one go

@thrix thrix force-pushed the init-toolhive branch 2 times, most recently from f7e48c3 to 34e03f7 Compare July 4, 2025 13:43
@thrix
Copy link
Author

thrix commented Jul 4, 2025

You only restored the maintainer entry, but did nothing to the package recipe.

I am sorry for my lack of understanding, it is my first contribution. What should be done to the package recipe please @acid-bong ?

Also an obligatory one: squash your edits, there should be one commit per addition or change (including maintainers), new packages/modules are created in one go

Done

@acid-bong
Copy link
Contributor

What should be done

First three from #420384 (review)

@thrix
Copy link
Author

thrix commented Jul 4, 2025 •
edited
Loading

What should be done

First three from #420384 (review)

Thank you, I guess I missed those when doing a rebase from my other laptop 🤦 I would swear I applied them before.

Should be applied now.

@thrix thrix requested a review from acid-bong July 4, 2025 23:57
@acid-bong
Copy link
Contributor

The code looks alright, but maintainer addition is supposed to be a separate commit, preceding everything else

@thrix
maintainers: add thrix
f620b06 
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
@thrix
Copy link
Author

thrix commented Jul 7, 2025

The code looks alright, but maintainer addition is supposed to be a separate commit, preceding everything else

Agreed, sorry, now I see where I got confused. should be done

acid-bong
acid-bong approved these changes Jul 7, 2025
View reviewed changes
Copy link
Contributor

@acid-bong acid-bong left a comment

Choose a reason for hiding this comment

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

Looks good so far. Welcome to the team and good luck nixing

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 7, 2025
@thrix
Copy link
Author

thrix commented Jul 7, 2025

Looks good so far. Welcome to the team and good luck nixing

Thanks for helping me out with my first contribution!

@thrix
Copy link
Author

thrix commented Jul 9, 2025

@acid-bong sorry for pinging again, I am just wondering when I could get see this merged? Do I need toget another review from somebody?

@acid-bong
Copy link
Contributor

No idea. I posted a link to this in a review request chat on Matrix, someone will visit you

@thrix
Copy link
Author

thrix commented Jul 28, 2025

Howdy, any update to get this merged?

@acid-bong
Copy link
Contributor

None yet, sadly

SuperSandro2000
SuperSandro2000 reviewed Jul 28, 2025
View reviewed changes
@thrix
Copy link
Author

thrix commented Aug 15, 2025

@SuperSandro2000 thanks for the review and sorry for the delay, I missed the notifications :) All addressed

@thrix thrix requested a review from SuperSandro2000 August 15, 2025 15:42
@thrix @acid-bong
toolhive: init at 0.0.47
00cc609 
Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Co-authored-by: Acid Bong <acidbong@tilde.club>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

1 more reviewer

@acid-bong acid-bong acid-bong approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thrix @acid-bong @SuperSandro2000