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

kata-ctl: Add the option to install kata-ctl to a user specified directory #6767

Merged
merged 1 commit into from
May 16, 2023

Conversation

ngpatel6
Copy link
Contributor

Fixes #5403

The Makefile was updated to use an INSTALL_PATH variable to track where the kata-ctl binary should be installed. If the user doesn't specify anything, then it uses the default path that cargo uses. Otherwise, it will install it in the directory that the user specified. The README.md file was also updated to show how to use the new option.

This work was done by pair programming by the following contributors: Cesar Tamayo
Kevin Mora Jimenez
Narendra Patel
Ray Karrenbauer
Srinath Duraisamy

Fixes #5403

@ngpatel6 ngpatel6 requested a review from a team as a code owner April 29, 2023 02:03
@katacontainersbot katacontainersbot added the size/tiny Smallest and simplest task label Apr 29, 2023
Copy link
Member

@amshinde amshinde left a comment

Choose a reason for hiding this comment

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

@ngpatel6 lgtm. You can attribute this commit to all the authors that worked on this by using multiple "Co-authored-by: " lines.
Take a look at: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

That way everyone who worked on the change can receive credit for their work. We have also accepted commits that have multiple "Signed-off-by: " lines in the past.

Copy link
Contributor

@cmaf cmaf left a comment

Choose a reason for hiding this comment

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

@ngpatel6 lgtm, pending @amshinde's suggestion to add co-authors

@amshinde amshinde added the hackathon short sprint projects label May 2, 2023
Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @ngpatel6.

lgtm

@@ -27,6 +27,11 @@ $ make
$ make install
```

If you would like to install the tool to a specific directory, then you can provide it through the `INSTALL_PATH` variable.
```bash
$ make install INSTALL_PATH=<install_path>
Copy link
Member

Choose a reason for hiding this comment

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

This line cause a static check error: ERROR: shell code in file 'src/tools/kata-ctl/README.md' is not valid.
Maybe use a real example will pass:

$ make install INSTALL_PATH=/opt/bin

@jodh-intel
Copy link
Contributor

Hi @ngpatel6 - The CI is failing due to the commit format:

Error: 

  7c2988e020f488c5556bb08ad3c6204f303305b9    Failed to find subsystem in subject

See the document below for help on formatting commits for the project.

https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format

Please can you git commit --amend and update the commit format to match the format specified in the doc above. Something like this:

kata-ctl: Allow INSTALL_PATH= to be specified

Update the kata-ctl install rule to allow it to be installed to a given directory

The Makefile was updated to use an `INSTALL_PATH` variable to track where the
kata-ctl binary should be installed.  If the user doesn't specify anything,
then it uses the default path that cargo uses.  Otherwise, it will install it
in the directory that the user specified.  The README.md file was also updated
to show how to use the new option.

This work was done by pair programming by the following contributors:
Cesar Tamayo
Kevin Mora Jimenez
Narendra Patel
Ray Karrenbauer
Srinath Duraisamy

Fixes: #5403.

Signed-off-by: Narendra Patel <narendra.g.patel@intel.com>

@jodh-intel jodh-intel closed this May 9, 2023
@jodh-intel jodh-intel reopened this May 9, 2023
@ngpatel6
Copy link
Contributor Author

ngpatel6 commented May 9, 2023

Apologies team, been busy with the normal workload after the hackathon. I'll be addressing these issues this week. Thanks for all the quick feedback!

@amshinde
Copy link
Member

@ngpatel6 Any updates on this PR? Would like to get this merged soon.

Update the kata-ctl install rule to allow it to be installed to a given directory

The Makefile was updated to use an INSTALL_PATH variable to track where the
kata-ctl binary should be installed.  If the user doesn't specify anything,
then it uses the default path that cargo uses.  Otherwise, it will install it
in the directory that the user specified.  The README.md file was also updated
to show how to use the new option.

Fixes kata-containers#5403

Co-authored-by: Cesar Tamayo <cesar.tamayo@intel.com>
Co-authored-by: Kevin Mora Jimenez <kevin.mora.jimenez@intel.com>
Co-authored-by: Narendra Patel <narendra.g.patel@intel.com>
Co-authored-by: Ray Karrenbauer <ray.karrenbauer@intel.com>
Co-authored-by: Srinath Duraisamy <srinath.duraisamy@intel.com>
Signed-off-by: Narendra Patel <narendra.g.patel@intel.com>
@ngpatel6
Copy link
Contributor Author

Apologies for the late update. I've made the suggested updates and it should be ready for a final review.

@chavafg
Copy link
Contributor

chavafg commented May 15, 2023

/test

@liubin liubin merged commit 47a02dc into kata-containers:main May 16, 2023
54 of 58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hackathon short sprint projects size/tiny Smallest and simplest task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kata-ctl: Improve install rule
7 participants