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

Added nix Flake check to flake.yml #323

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Added nix Flake check to flake.yml #323

merged 2 commits into from
Sep 15, 2023

Conversation

9glenda
Copy link
Member

@9glenda 9glenda commented Sep 14, 2023

The nix flake check Command Tests rather the nix package builds in all supported systems and checks formatting.
This is a follow up to #293. There I was stating that shellcheck is executed through github actions which is not the case because of only running nix build.

@9glenda
Copy link
Member Author

9glenda commented Sep 14, 2023

I will force push with a new commit message and then convert to a normal pull request

@9glenda 9glenda mentioned this pull request Sep 14, 2023
12 tasks
@cafkafk cafkafk self-requested a review September 14, 2023 16:59
@9glenda
Copy link
Member Author

9glenda commented Sep 14, 2023

This will add extra time for the checks to run since it's testing way more then just the nix build. But in my opinion that's not as important as ensuring the code does not accidentally break something

@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

This will add extra time for the checks to run since it's testing way more then just the nix build. But in my opinion that's not as important as ensuring the code does not accidentally break something

I mean we already have quite a heavy setup with unit-tests right now, so it probably won't be much of a difference.

@9glenda
Copy link
Member Author

9glenda commented Sep 14, 2023

This will add extra time for the checks to run since it's testing way more then just the nix build. But in my opinion that's not as important as ensuring the code does not accidentally break something

I mean we already have quite a heavy setup with unit-tests right now, so it probably won't be much of a difference.

In my personal projects with heavy tests I used the github auto merge feature maybe you want to take a look at it but there could be issues with the flake.yml action not being a check and just a github action

@9glenda 9glenda marked this pull request as ready for review September 14, 2023 17:18
@9glenda
Copy link
Member Author

9glenda commented Sep 14, 2023

Time reported by the github action:

Action Time
nix build 4min
nix flake check 4min

which means 4min extra time. This should be fine. optionally we could remove the nix build because nix flake check also ensures building of the packages (with --all-systems on all supported targets). Or alternatively add --no-build to not build during the nix flake check.

The nix flake check Command Tests rather the nix package builds in all supported systems and checks formatting.

Signed-off-by: naŭ glenda <plan9git@proton.me>
@cafkafk
Copy link
Member

cafkafk commented Sep 14, 2023

optionally we could remove the nix build because nix flake check also ensures building of the packages (with --all-systems on all supported targets)

I mean this sounds sensible to do if we don't get anything out of build but waiting time.

@9glenda
Copy link
Member Author

9glenda commented Sep 14, 2023

optionally we could remove the nix build because nix flake check also ensures building of the packages (with --all-systems on all supported targets)

I mean this sounds sensible to do if we don't get anything out of build but waiting time.

Ok I'm going to remove nix build

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for the contribution, merging 👍

@cafkafk cafkafk merged commit b98b3a1 into eza-community:main Sep 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants