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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

kube3d: increase discoverability #99563

Merged
merged 4 commits into from Nov 26, 2020
Merged

Conversation

@06kellyjac
Copy link
Contributor

@06kellyjac 06kellyjac commented Oct 4, 2020

Motivation for this change

I've searched for "k3d" previously using https://search.nixos.org but kube3d didn't show up so I've been maitaining my own copy of it for months 馃槄
Mentioning "k3d" in the description and longDescription should make it easier to find.

Things done

nixpkgs-fmted the file to fit with contributing guidelines.

Mentioned k3d in the description and longDescription

https://search.nixos.org can find things from either description but nix search just looks at the short description.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (x86_64)
    • [ ] macOS Not on the platforms list
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@06kellyjac 06kellyjac force-pushed the 06kellyjac:kube3d_discoverability branch from 0d6c1f3 to 3461263 Oct 4, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 22, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/346

@06kellyjac 06kellyjac force-pushed the 06kellyjac:kube3d_discoverability branch from 3461263 to 9b58a5a Nov 17, 2020
@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 17, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Nov 17, 2020
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 17, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot added the needs_reviewer label Nov 17, 2020
@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 17, 2020

Result of nixpkgs-review pr 99563 1

1 package built:
  • kube3d
@jlesquembre
Copy link
Contributor

@jlesquembre jlesquembre commented Nov 18, 2020

@06kellyjac thanks for the PR, I like your changes, but I have 2 questions:

  • Why did you convert buildFlagsArray to an array?
  • Why did you replaced stdenv.lib with lib?

About the first question, I found this related issue: #99200 but I still don't understand the reason.

I'm not opposed to do that changes, I just want to understand the motivation behind them

@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 18, 2020

Why did you convert buildFlagsArray to an array?

IMO it makes more sense for me that something called an "Array" actually be an array. I'm not sure if there are any other advantages or disadvantages that are more substantial.
I've never had any issues like the one you mentioned. Just from scanning it I'm not sure what's wrong with the concat output

位 BOB=("a" "b" "c")
位 echo "${BOB[@]}"
a b c
位 BUILDFLAGSARRAY=("-tags" "netgo" "-ldflags='-w -X PACKAGE.gitTag=VERSION'")
位 echo "${BUILDFLAGSARRAY[@]}" # notwrapped in 'quotes'
-tags netgo -ldflags='-w -X PACKAGE.gitTag=VERSION'

-tags netgo -ldflags='-w -X PACKAGE.gitTag=VERSION' looks fine to me, and at the very least it's working for k3d

Why did you replaced stdenv.lib with lib?

If stdenv is only being used for stdenv.lib then why not just grab lib directly like many other golang packages. If stdenv was being used for additional things it'd make sense to keep it like it was.

@jlesquembre
Copy link
Contributor

@jlesquembre jlesquembre commented Nov 18, 2020

IMO it makes more sense for me that something called an "Array" actually be an array.

Totally agree with that, it's only that in the examples I remember, it was always a string. And if it ain't broke, don't fix it.
But not idea if there is a recommended way for that, I didn't find anything in the docs.

If stdenv is only being used for stdenv.lib then why not just grab lib directly like many other golang packages.

It makes sense to me, I only wanted to know if there are some best practices about it.

To me, it's ok to merge your changes, I just wanted to know if there are some guide lines about that topics

@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 18, 2020

Thanks for looking it all over

I just wanted to know if there are some guide lines about that topics

No worries. I've not been using and packaging for nix too long so I'm also trying to work out some of the best practices 馃槄

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 21, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@jlesquembre
Copy link
Contributor

@jlesquembre jlesquembre commented Nov 23, 2020

I tested the executable and all looks good.
/status needs_merger

@marvin-mk2 marvin-mk2 bot added needs_merger and removed awaiting_reviewer labels Nov 23, 2020
@marvin-mk2 marvin-mk2 bot requested a review from kevincox Nov 23, 2020
@marvin-mk2 marvin-mk2 bot added awaiting_merger and removed needs_merger labels Nov 23, 2020
homepage = "https://github.com/rancher/k3d";
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container";
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container - k3d";

This comment has been minimized.

@SuperSandro2000

SuperSandro2000 Nov 23, 2020
Contributor

Suggested change
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container - k3d";
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container";

This comment has been minimized.

@06kellyjac

06kellyjac Nov 23, 2020
Author Contributor

I added k3d to the short description because nix search uses description but doesn't look at longDescription

nix search seems to only look at the short description so it's worth
including there too.

06kellyjac@83cd754

This comment has been minimized.

@kevincox

kevincox Nov 24, 2020
Contributor

I think this is fine with me. Sound good to leave it @SuperSandro2000?

homepage = "https://github.com/rancher/k3d";
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container";
description = "A helper to run k3s (Lightweight Kubernetes. 5 less than k8s) in a docker container - k3d";

This comment has been minimized.

@kevincox

kevincox Nov 24, 2020
Contributor

I think this is fine with me. Sound good to leave it @SuperSandro2000?

@SuperSandro2000
Copy link
Contributor

@SuperSandro2000 SuperSandro2000 commented Nov 26, 2020

Can you please resolve the merge conflict?

06kellyjac added 4 commits Oct 4, 2020
search.nixos.org was able to show me `deno` when searching for
"executable" which was in the longDescription
While I was able to find `deno` by searching for "executable" from it's
longDescription on search.nixos.org, I couldn't find it using `nix search`

`nix search` seems to only look at the short description so it's worth
including there too.
Moved k3sVersion to be a variable.
Converted buildFlagsArray to an array
Moved vendorSha256 closer to src
Moved doCheck between build and install related bits (like where the
phase happens)
Replaced stdenv.lib with lib
@06kellyjac 06kellyjac force-pushed the 06kellyjac:kube3d_discoverability branch from 9b58a5a to cdfa9b0 Nov 26, 2020
@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 26, 2020

Done

@kevincox kevincox merged commit a5da6e5 into NixOS:master Nov 26, 2020
19 checks passed
19 checks passed
tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="cdfa9b0"; rev="cdfa9b0c26184fc6850a5f27ab14514427f3a87a"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
kube3d, kube3d.passthru.tests on aarch64-linux Success
Details
kube3d, kube3d.passthru.tests on x86_64-linux Success
Details
@06kellyjac 06kellyjac deleted the 06kellyjac:kube3d_discoverability branch Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.