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

Jump to bottom

_3llo: init at 0.3.0 #65370

Merged
merged 2 commits into from Aug 25, 2019
Merged

_3llo: init at 0.3.0 #65370

merged 2 commits into from Aug 25, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jul 25, 2019 •
edited

Motivation for this change

Simple CLI client for trello.com. It can be used like this:

$ export TRELLO_USER=your_username
$ export TRELLO_KEY=your_key
$ export TRELLO_TOKEN=your_token
$ ./result/bin/3llo

I didn't create a module for this as I don't think that those secrets
should live in the Nix store. Ideally 3llo can be used from a script
which retrieves secrets from some kind of password store like this:

export TRELLO_KEY=$(pass show trello/key)
export TRELLO_TOKEN=$(pass show trello/token)
3llo $@
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • 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 nix-review --run "nix-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.

@worldofpeace
Copy link
Contributor

Why is there an underscore in the attribute name when the repo doesn't name the tool that way?

worldofpeace
worldofpeace suggested changes Jul 27, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Expression looks fine to me.

Not sure about its place in nixpkgs though.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@worldofpeace
Copy link
Contributor

It would be nice if this program self documented itself instead of erroring ungracefully when you don't set the aforementioned environment variables

❯ ./result/bin/3llo --help
Traceback (most recent call last):
	4: from ./result/bin/3llo:18:in `<main>'
	3: from ./result/bin/3llo:18:in `load'
	2: from /nix/store/dsqvych6dja9rvb2ar4fj914zhlwykl4-3llo-0.3.0/lib/ruby/gems/2.5.0/gems/3llo-0.3.0/bin/3llo:14:in `<top (required)>'
	1: from /nix/store/dsqvych6dja9rvb2ar4fj914zhlwykl4-3llo-0.3.0/lib/ruby/gems/2.5.0/gems/3llo-0.3.0/bin/3llo:14:in `fetch'
/nix/store/dsqvych6dja9rvb2ar4fj914zhlwykl4-3llo-0.3.0/lib/ruby/gems/2.5.0/gems/3llo-0.3.0/bin/3llo:14:in `block in <top (required)>': Have you set TRELLO_USER? (RuntimeError)

@Ma27
Copy link
Member Author

Ma27 commented Jul 27, 2019

Why is there an underscore in the attribute name when the repo doesn't name the tool that way?

Nix doesn't like attribute names with a number at first. I was unsure how to deal with this and I just used this pattern from other tools like _1password.

It would be nice if this program self documented itself instead of erroring ungracefully when you don't set the aforementioned environment variables

In fact you're right. Not sure if I can make it today, but I'll prepare an upstream patch for this (unless there's something that just didn't land in the release :) )

Would this be a better fit for tools since it's such a small program?

Hmm, probably yes.... I'd like to note though that I actually dislike this section-based approach as it makes it IMHO way harder to find an appropriate place for a new package and I honestly doubt that many people benefit from this... at least whenever I look for something I either grep over nixpkgs or all-packages.nix.

@worldofpeace
Copy link
Contributor

Why is there an underscore in the attribute name when the repo doesn't name the tool that way?

Nix doesn't like attribute names with a number at first. I was unsure how to deal with this and I just used this pattern from other tools like _1password.

I think we can just make it "3llo" in all-packages.nix.

It would be nice if this program self documented itself instead of erroring ungracefully when you don't set the aforementioned environment variables

In fact you're right. Not sure if I can make it today, but I'll prepare an upstream patch for this (unless there's something that just didn't land in the release :) )

That would be really excellent if you did ✨

Would this be a better fit for tools since it's such a small program?
Hmm, probably yes.... I'd like to note though that I actually dislike this section-based approach as it makes it IMHO way harder to find an appropriate place for a new package and I honestly doubt that many people benefit from this... at least whenever I look for something I either grep over nixpkgs or all-packages.nix.

I think it helps with breaking up the psuedo alphabetization. Then at least you can see it restarting
at least in definite sections.

@Ma27
Copy link
Member Author

Ma27 commented Jul 27, 2019

I think we can just make it "3llo" in all-packages.nix.

But isn't that counter-intuitive when you want to add a bunch of packages like with pkgs; [ foo bar ] where you'd have to reference 3llo with pkgs."3llo"? (I'm surprised that nix-build -A 3llo would work btw).

@worldofpeace
Copy link
Contributor

I think we can just make it "3llo" in all-packages.nix.

But isn't that counter-intuitive when you want to add a bunch of packages like with pkgs; [ foo bar ] where you'd have to reference 3llo with pkgs."3llo"? (I'm surprised that nix-build -A 3llo would work btw).

Huh, we would then get a syntax error when referencing it in the function arguments of a package

syntax error, unexpected '"', expecting '}'

I don't think there's a guideline in the Nixpkgs manual about this, rather unfortunate that it would have to be _3llo.

@Ma27
Copy link
Member Author

Ma27 commented Jul 28, 2019

@worldofpeace updated. Would be awesome if you could review the upstream patch as well. Please keep in mind that I had to change a minor detail in bundlerApp. I'm currently running nix-review, but I definetely need more reviewers then :)

@worldofpeace
Copy link
Contributor

Gave it a review @Ma27

@Ma27
_3llo: init at 0.3.0
0ad3568 
Simple CLI client for `trello.com`. It can be used like this:

```
$ export TRELLO_USER=your_username
$ export TRELLO_KEY=your_key
$ export TRELLO_TOKEN=your_token
$ ./result/bin/3llo
```

I didn't create a module for this as I don't think that those secrets
should live in the Nix store. Ideally `3llo` can be used from a script
which retrieves secrets from some kind of password store like this:

```
export TRELLO_KEY=$(pass show trello/key)
export TRELLO_TOKEN=$(pass show trello/token)
3llo $@
```
@Ma27
ruby/bundlerApp: ignore gemset in env attribute set
ebc4a41 
The `gemset` field can be an attribute set as well in `buildRubyGem`,
however attribute sets can't be coerced to strings for the environment
set.

The impact should be relatively small as the environment variables are
only used by the `runCommand` script for `bunlderApp` which doesn't
refer to `gemset` at all.
@Ma27
Copy link
Member Author

Ma27 commented Aug 19, 2019

@worldofpeace updated the patch :)

worldofpeace
worldofpeace approved these changes Aug 25, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Ma27.

@worldofpeace worldofpeace merged commit 741163e into NixOS:master Aug 25, 2019
@Ma27 Ma27 deleted the package-3llo branch August 25, 2019 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

@alyssais alyssais Awaiting requested review from alyssais

@zimbatm zimbatm Awaiting requested review from zimbatm

Assignees
No one assigned
Labels
6.topic: ruby 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Ma27 @worldofpeace