-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
bitrise: init at 2.19.0 #332816
bitrise: init at 2.19.0 #332816
Conversation
8e5fbf7
to
4802a4f
Compare
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/4388 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
Just a few suggestions. Please also be sure to separate your change to maintainer-list.nix
to a commit maintainers: add ofalvai
and re-arrange the order, so that commit will come first.
pkgs/by-name/bi/bitrise/package.nix
Outdated
hash = "sha256-VjuDeRl/rqA7bdhn9REdxdjRon5WxHkFIveOYNpQqa8="; | ||
}; | ||
|
||
doCheck = false; # many tests rely on writing to $HOME/.bitrise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do something like:
preCheck = "export HOME=`mktemp -d`";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I didn't think of this trick. I'd prefer merging this first though, then I'll take another stab at running the tests later if that's okay for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started working around the failing tests, but after patching $HOME, I found out that some tests rely on network access, so this is going be a longer journey
Thank you for the suggestions @gador, I believe I fixed them |
pkgs/by-name/bi/bitrise/package.nix
Outdated
hash = "sha256-VjuDeRl/rqA7bdhn9REdxdjRon5WxHkFIveOYNpQqa8="; | ||
}; | ||
|
||
doCheck = false; # many tests rely on writing to $HOME/.bitrise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doCheck = false; # many tests rely on writing to $HOME/.bitrise | |
# many tests rely on writable $HOME/.bitrise and require network access | |
doCheck = false; |
pkgs/by-name/bi/bitrise/package.nix
Outdated
passthru.updateScript = nix-update-script { }; | ||
|
||
meta = { | ||
changelog = "https://github.com/bitrise-io/bitrise/releases/tag/${src.rev}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pandapip1 is there anything solved about parametric changelogs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend doing
changelog = "https://github.com/bitrise-io/bitrise/releases/tag/${src.rev}"; | |
changelog = "https://github.com/bitrise-io/bitrise/releases"; |
as this will display the full changelog instead of the abbreviated version that only shows the changes for a specific release.
pkgs/by-name/bi/bitrise/package.nix
Outdated
|
||
meta = { | ||
changelog = "https://github.com/bitrise-io/bitrise/releases/tag/${src.rev}"; | ||
description = "Bitrise runner CLI - run your automations on your Mac or Linux machine"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = "Bitrise runner CLI - run your automations on your Mac or Linux machine"; | |
description = "CLI for running your Workflows from Bitrise on your local machine"; |
pkgs/by-name/bi/bitrise/package.nix
Outdated
meta = { | ||
changelog = "https://github.com/bitrise-io/bitrise/releases/tag/${src.rev}"; | ||
description = "Bitrise runner CLI - run your automations on your Mac or Linux machine"; | ||
homepage = "https://bitrise.io"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
homepage = "https://bitrise.io"; | |
homepage = "https://bitrise.io/cli"; |
@AndersonTorres I applied all your suggestions, thank you! Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember me testing the package later, I will sleep a bit now.
Besides, visually LGTM
# many tests rely on writable $HOME/.bitrise and require network access | ||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it feasible to selectively disable the network tests? Some tests are still better than none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already have a draft PR for that, but the list of ignored tests is quite long: https://github.com/NixOS/nixpkgs/pull/333648/files#diff-6ed507756f46cf318b766f9e189a33a2c87be833e2ca6458c13b6fa43ff714dcR18
I'd rather do the clean up and refactor of the tests in the repo itself (there are distinct integration tests already, I think these failing tests should be tagged as integration tests). But I'd prefer merging this first PR with doCheck = false
, and then iterate on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fine with me 👍
Small nitpick: Second commit should be named |
This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 1 package built:
|
@gador whoops, commit message fixed |
This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch). Result of 1 package built:
|
Description of changes
Bitrise CLI repo: https://github.com/bitrise-io/bitrise
I've been using this package locally for a while, decided to upstream it.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.