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

kns: init @ 86502949c31432bd95895cfb26d1c5893c533d5c #193995

Closed
wants to merge 9 commits into from

Conversation

crutonjohn
Copy link
Contributor

@crutonjohn crutonjohn commented Oct 1, 2022

Description of changes

Add new package for kns the easy kubernetes namespace switcher. Upstream can be found here: https://github.com/blendle/kns

kns is a shell script that uses fzf to help navigate namespaces within a kubernetes cluster.

I used the existing kubectl-node-shell to base my work on, as they are both fancy bash scripts.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@crutonjohn crutonjohn requested a review from dit7ya October 5, 2022 13:08
@dit7ya
Copy link
Member

dit7ya commented Oct 10, 2022

Result of nixpkgs-review pr 193995 run on x86_64-linux 1

1 package failed to build:
  • kns

@crutonjohn
Copy link
Contributor Author

crutonjohn commented Jan 24, 2023

i had to change the rev in fetchFromGithub such that it pointed to a full commit hash. it should also work if we just "pin" it to master as well if that aligns with how we should proceed with this. there is no release strategy or anything for this project so i'm not able to pin on a tag/release.

EDIT:

it should also work if we just "pin" it to master as well if that aligns with how we should proceed with this.

i just realized that this won't work because every new commit will result in a new sha256 sum, breaking this pkg as a result.

pkgs/applications/networking/cluster/kns/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/cluster/kns/default.nix Outdated Show resolved Hide resolved
sha256 = "sha256-zcJaH+Uyc/rCDRlQi+lSKaG7z4VWlNZ13klDnvL3Jgc=";
};

strictDeps = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

buildInputs = [ fzf kubectl ];

pkgs/applications/networking/cluster/kns/default.nix Outdated Show resolved Hide resolved
@mmlb
Copy link
Contributor

mmlb commented Jul 5, 2023

@crutonjohn can you take a look at the changes I suggested? I'd be happy to take over the PR if you'd like too.

@crutonjohn
Copy link
Contributor Author

@crutonjohn can you take a look at the changes I suggested? I'd be happy to take over the PR if you'd like too.

these all look good to me, I've been on holiday with my family but happy to take care of this sometime this week.

@mmlb
Copy link
Contributor

mmlb commented Jul 6, 2023

@crutonjohn can you take a look at the changes I suggested? I'd be happy to take over the PR if you'd like too.

these all look good to me, I've been on holiday with my family but happy to take care of this sometime this week.

awesome, looking forward to getting rid of this from all my shell.nix/flake.nix files :D

@crutonjohn crutonjohn requested a review from mmlb July 6, 2023 22:07
Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

lgtm, probably want to retitle the PR to kns: init @ 8650294 though

@crutonjohn crutonjohn changed the title kns: init @ 50b6370c88136a580599c913f0fae2cb24981dc1 kns: init @ 86502949c31432bd95895cfb26d1c5893c533d5c Jul 10, 2023
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for your contribution.

I've left some comments, left me know if this is clear enough.

Also, there should only be 2 commits in this PR:

  • 1 commit for adding you to maintainers: maintainers: add crutonjohn
  • 1 commit for the rest: kns: init at unstable-2022-04-25

Thanks!

@@ -0,0 +1,35 @@
{ stdenvNoCC, lib, fetchFromGitHub, fzf, kubectl }:

stdenvNoCC.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

The finalAttrs pattern will let you remove the rec keyword (see its implementation in Nix).

Why is this pattern preferred to rec ?

Let's take this simple code example:

mkDerivation rec {
   foo = 1;
   bar = foo + 1;
}

and then .overrideAttrs(old: { foo = 2; }), you'll get { foo = 2; bar = 2; } while with finalAttrs pattern, it would work correctly because it's a real fixed point.

Let me share a couple of useful links regarding the finalAttrs pattern:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

Feel free to reach out if you need some assistance.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I get rid of repo = pname then I don't need rec at all, nor finalAttrs, 2 birds with one stone!

Copy link
Contributor

Choose a reason for hiding this comment

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

Bingo ! :)


src = fetchFromGitHub {
owner = "blendle";
repo = pname;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use this.

repo = pname; is nice for DRY but creates a binding that goes too far.

See further information about this here: nix-community/nixpkgs-lint#21

@mmlb
Copy link
Contributor

mmlb commented Aug 17, 2023

@crutonjohn do you have the bw to apply @drupol's suggestions? If not let me know and I'll take over (alas in a separate PR).

@crutonjohn
Copy link
Contributor Author

@crutonjohn do you have the bw to apply @drupol's suggestions? If not let me know and I'll take over (alas in a separate PR).

yeah ive been back and forth traveling for work so im tapped out. would be incredibly grateful if you could just knock it out. 🙏

@crutonjohn crutonjohn closed this Aug 17, 2023
@crutonjohn
Copy link
Contributor Author

@crutonjohn do you have the bw to apply @drupol's suggestions? If not let me know and I'll take over (alas in a separate PR).

yeah ive been back and forth traveling for work so im tapped out. would be incredibly grateful if you could just knock it out. 🙏

@mmlb will you tag me in the new PR plz

@mmlb mmlb mentioned this pull request Aug 17, 2023
6 tasks
@crutonjohn crutonjohn deleted the feat/add-kns branch February 9, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants