k8sgpt: 0.3.30 -> 0.3.41; move to by-name

[mrgiles]

Things done

This commit updates k8sgpt to release 0.3.41

Release: v0.3.41

Note:
On my local system, the command nixpkgs-review passed the buildPhase but failed the checkPhase (trivy integration test). I wanted to try and see if I can run the nixpkgs-review command pointing to this PR, which is one of the options available. I will test this first with a draft PR.

This is the local build log file: k8sgpt.log.

• 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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 24.11 Release Notes (or backporting 23.11 and 24.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.

[mrgiles]

The nixpkgs-review command using the PR also failed. Maybe somebody else can run nixpkgs-review on their system and see if it succeeds? Thanks.

[mrgiles]

After doing some troubleshooting and research focused on the build issue, here' what I got.

The error I got (it's in the attached log file):

Running phase: checkPhase
...
panic: Get "https://aquasecurity.github.io/helm-charts/index.yaml": dial tcp: lookup aquasecurity.github.io on [::1]:53: read udp [::1]:47967->[::1]:53: read: connection refused

This is related on how Go is resolving Internet host names. I tested several DNS client configurations, with and wothout stub resolver, with and without IPv6, etc, I couldn't figure out a working solution. So I forced Go to use the 8.8.8.8 resolver by adding a preBuildPhases section to the build file for my build tests and this solved the issue for me.

This is the code added, it's not included in this PR because this error might just happen on my system. But, if you think it needs to be included, I can add it.

  preBuildPhases = [ "setDNS" ];
  setDNS = ''
    export GODNS=8.8.8.8
    export GODEBUG=netdns=go
  '';

I'm attaching the k8sgpt.log here but basically, now it shows that the checkPhase runs successfully and it builds the k8sgpt binary:

buildPhase completed in 2 minutes 9 seconds
checkPhase completed in 2 minutes 0 seconds

Result of nixpkgs-review run on x86_64-linux 1

1 package built:

• k8sgpt

@developer-guy, @kranurag7, what do you recommend doing? I thought of opening an upstream issue and see if anybody else is having this issue but, when I tested it outside Nix, just using their Makefile, it worked. So a Nix issue might not be relevant to them. Thanks

[kranurag7]

I know nix sets CGO_ENABLED=1 out of the box and we are explicitly setting it to CGO_ENABLED = 0 so not sure if removing this would help here. Can you please check this out? Generally using the default is preferred which is CGO_ENABLED=1.

ref: https://nixos.org/manual/nixpkgs/stable/#var-go-CGO_ENABLED

[mrgiles]

Thanks for checking @kranurag7.

I tried with CGO_ENABLED = 1.
The build failed very early on. It didn't even start the Running Phase:... portion, so no logs.

Applying `nixpkgs` diff...
error: patch failed: pkgs/applications/networking/cluster/k8sgpt/default.nix:13
error: pkgs/applications/networking/cluster/k8sgpt/default.nix: patch does not apply

[mrgiles]

I've also tried removing the CGO_ENABLED = 0; line to see if it made any difference but I've got the same result.

[mrgiles]

I'm going to move it to Ready for review, to see if it passes the automatic online build process, since it may just fail on my local system.

[kranurag7]

I'm going to move it to Ready for review, to see if it passes the automatic online build process, since it may just fail on my local system.

Thanks for working on it. Looks like the checks are passing.
If it still doesn't work on your system which you can check by fetching the binary from CI cache then we can export the DNS specific variables which seems to be working on your system and won't be breaking for others as well.

One more request, can you please add yourself as maintainer here, at this point, you've really gone deep in order to fix the issue and understand it better than me.

[mrgiles]

Hi @kranurag7. I've run several tests on my system, including setting up different stub resolvers and DNS configurations based on the error that I get during the checkPhase of the package build but no luck, so far.

I'll open an issue upstream and ask the developers of k8sgpt. I've looked on their repo but couldn't find a similar issue. I 'd like to have all the facts before adding those variables to the nix file.

I'll continue working on this tomorrow, and I'll add myself as maintainer. Thanks.

[mrgiles]

I've been testing different solutions. I think that skipping the trivy test is less disruptive than introducing those Go variables that I proposed before. I can confirm that the k8sgpt binary builds locally and that it works, including the trivy integration.

I'm stuck now with failing new nixpkgs-review tests because of this PR that I've just found while looking for why my tests were failing. Once this is solved, I will re-run the build tests and post the results.

I didn't post the issue upstream because I tested building k8sgpt directly from their repo and it works, so it seems like it's a Nix packages build issue.

[mrgiles]

They solved the lisp-modules issue. Now the k8sgpt package builds fine.

See build log file: k8sgpt-x86_64-linux.log

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 348479

---

x86_64-linux

✅ 1 package built:

• k8sgpt

[kranurag7]

Thank you for sticking through it @mrgiles I trust you and happy to get the changes in. 🥇

[kranurag7]

@NixOS/nixpkgs-merge-bot merge

[nixpkgs-merge-bot]

@kranurag7 merge not permitted (#305350):
pkgs/applications/networking/cluster/k8sgpt/default.nix is not in pkgs/by-name/

[kranurag7]

looks like I cannot merge this patch, I'm sure nixpkgs maintainer will take another looks and get this changes in. Thanks again for spending your time on this during the weekend. I hope this works smooth now.

[mrgiles]

Hey @kranurag7, thanks for checking.

I tried to fix the merge error myself (based on the error that you got when merging) by moving the package to pkgs/by-name and made a mess. I left everything as it was before the latest change. Oops!

I hope the Nix Packages maintainer can fix this one.
Do you need to run the merge command again?

[kranurag7]

@NixOS/nixpkgs-merge-bot merge

[nixpkgs-merge-bot]

@kranurag7 merge not permitted (#305350):
pkgs/applications/networking/cluster/k8sgpt/default.nix is not in pkgs/by-name/

[kranurag7]

Thanks for your work @mrgiles let's leave this to nixpkgs maintainer for merging it. I hope you get a good sleep now. This was quite a lot of debugging for a go package but kudos you made it to the end. :)

[mrgiles]

Thank you for your comments @gepbird! They helped me to better understand the process.
Now I can see more clearly which of my assumptions were correct and which were wrong.
I've restored the changes that I made before to move the package under by-name.
Let's give this another try!

[gepbird]

Great, the by-name change looks good!

Again, please recreate your commits according to https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions.
What I mean by this you should have for example commits with these messages and delete your previous commits:

• k8sgpt: move to by-name
• k8sgpt: format
• k8sgpt: 0.3.30 -> 0.3.41
• k8sgpt: add maintainer mrgiles

If you're unsure how to do that, you can search for "git rebase" on the internet and learn it. At last resort squash your commits into a single one, you can find the instructions for that on the aforementioned link.

[emilazy]

I haven’t reviewed this PR but I just wanted to clarify a little:

In a future PR where k8sgpt is already in by-name and you make a version bump for example, you can use the nixpkgs-merge-bot to merge it yourself.

This only applies to automatic version bumps by @r-ryantm. Currently a committer’s approval is required for all human‐authored changes, so a PR like this one would not be eligible at this time even if the package was already in pkgs/by-name. Allowing package maintainers to merge automatic updates to pkgs/by-name packages is a very restrictive trial programme and can’t be used for anything other than trivial automatic version bumps.

[mrgiles]

I've renamed the commits as requested. Thanks for reviewing @gepbird

[gepbird]

Your first commit which updates the package also copies the package to by-name, your second add maintainer commit makes unrelated changes to the package and removes the previous commit's by-name addition, and the 3rd move to by-name commit looks good.

Please fix the first two commits, or squash everything to one commit on the last resort.

[mrgiles]

Let's see if this works. I've started it from scratch this time. Thanks.

[mrgiles]

Maybe a better approach to preserve the history of changes to the package would be to create 2 separate PRs.
The first one to just upgrade the package, and the second one to move it to by-name.
What do you think?

[gepbird]

Maybe a better approach to preserve the history of changes to the package would be to create 2 separate PRs.
The first one to just upgrade the package, and the second one to move it to by-name.
What do you think?

It's preferred to do all the things for a package in one PR (unless that change gets very big/complex). Updating a package and moving it to by-name is considered a simpler change. It's very common for a package to get updated, formatted, refactored in one PR.

Multiple PRs can complicate things for you and the reviewers. For example if you have PR B that depends on PR A, you either put all PR A's commits into PR B, but then you need to change something in PR A, and you need to also update PR B for that. Or if you only open PR B after PR A has been merged, the overall merge time will most likely be longer.

[gepbird]

Let's see if this works. I've started it from scratch this time. Thanks.

Commits look at first sight, thanks! I'll review this soon.

[gepbird]

Tested the build and binary on x86_64-linux, changes LGTM!

[gepbird]

cc @kranurag7 @developer-guy

[kranurag7]

Thanks for driving this @mrgiles

[mrgiles]

Thanks for reviewing and testing @gepbird!
I've learned a lot from your constructive feedback.

FYI @willbush

[Aleksanaa]

You don't have to write this