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

feat(chores): Add missing tools to nix-shell #352

Merged
merged 16 commits into from
Dec 16, 2024

Conversation

mhkarimi1383
Copy link
Contributor

@mhkarimi1383 mhkarimi1383 commented Dec 10, 2024

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?: controller-tools and some other tools are missing from nix-shell

What this PR does?: adds corresponding packages

Does this PR require any upgrade changes?: No

If the changes in this PR are manually verified, list down the scenarios covered::

just run nix-shell

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.22%. Comparing base (8feeaa0) to head (0313fa2).
Report is 39 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #352      +/-   ##
===========================================
- Coverage    98.66%   97.22%   -1.44%     
===========================================
  Files            2        2              
  Lines          673      902     +229     
===========================================
+ Hits           664      877     +213     
- Misses           5       18      +13     
- Partials         4        7       +3     
Flag Coverage Δ
bddtests 97.22% <ø> (-1.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhkarimi1383
Copy link
Contributor Author

I also want to add .envrc to automatically load that

@tiagolobocastro
Copy link

Pardon my ignorance, what's in kubernetes-controller-tools ?

@mhkarimi1383
Copy link
Contributor Author

@tiagolobocastro
controller-gen cli

@dsharma-dc
Copy link
Contributor

dsharma-dc commented Dec 10, 2024

Though nothing harmful, do we really need this in product nix shell ? The required controller-gen is currently installed from the Makefile in the repo and is used to generate manifests.

@mhkarimi1383
Copy link
Contributor Author

In my NixOS I ran make -j 1 and controller-gen not found and with the latest one from nix repos (unstable) I got unrecognized option error for controller-gen command

@tiagolobocastro
Copy link

Though nothing harmful, do we really need this in product nix shell ? The required controller-gen is currently installed from the Makefile in the repo and is used to generate manifests.

Downloaded binaries are not likely to work on nix and NixOS, may require patching et all.
It's simpler to just put it on the nix-shell rather than figure out why it doesn't work.

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 force-pushed the add-controller-tools-nix branch from 27a47cd to 73fce9d Compare December 11, 2024 06:31
@mhkarimi1383 mhkarimi1383 changed the title feat(chores): Add kubernetes-controller-tools to nix-shell feat(chores): Add missing tools to nix-shell Dec 11, 2024
@mhkarimi1383 mhkarimi1383 marked this pull request as draft December 11, 2024 07:42
@mhkarimi1383
Copy link
Contributor Author

I made this PR draft since I see that some more tools are missing....

I want to add every required tools to shell.nix to make easier to work for NixOS users :)

NOTE: after upgrading golang version we have to update this file,
also updating nixpkgs will break this since go1.19 is not available to
newer versions of nixpkgs

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 marked this pull request as ready for review December 11, 2024 08:16
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 force-pushed the add-controller-tools-nix branch from 4bfbd2c to 7757f8a Compare December 11, 2024 08:18
Our nixpkgs is a bit old

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383
Copy link
Contributor Author

Commit notes/descriptions are important

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383
Copy link
Contributor Author

Also we can change all of the actions to use nix shell, so changing go version, etc., will be easier

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@tiagolobocastro
Copy link

Sounds good, just let us know when this is working and ready to merge 👍

@mhkarimi1383
Copy link
Contributor Author

mhkarimi1383 commented Dec 11, 2024

@tiagolobocastro
Looks like ready, But since I'm getting #353 error I can't be sure about that

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
coverage.txt Outdated Show resolved Hide resolved
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383
Copy link
Contributor Author

Almost complete,
I have isolated nix-shell's gopath and tempdir, Since installing specific version of some packages is a bit messy

Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
shell.nix Outdated Show resolved Hide resolved
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@mhkarimi1383 mhkarimi1383 force-pushed the add-controller-tools-nix branch from 14f7ad5 to 1dbb827 Compare December 14, 2024 16:09
Signed-off-by: Muhammed Hussein Karimi <info@karimi.dev>
@dsharma-dc dsharma-dc merged commit 5c68557 into openebs:develop Dec 16, 2024
5 checks passed
@mhkarimi1383 mhkarimi1383 deleted the add-controller-tools-nix branch December 16, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants