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.

Sign up for GitHub

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

feat: Introduce kraft lib rm #631

Merged
merged 1 commit into from
Dec 17, 2023
Merged

feat: Introduce kraft lib rm #631

merged 1 commit into from
Dec 17, 2023

Conversation

MdSahil-oss
Copy link
Contributor

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • [] Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

The command kraft pkg remove removes library from the project directory and updates kraftfile (Opposite of kraft pkg add )

craciunoiuc
craciunoiuc approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

Some comments from my side 😅

unikraft/app/application.go Outdated Show resolved Hide resolved
unikraft/app/application.go Outdated Show resolved Hide resolved
@craciunoiuc craciunoiuc changed the title feat: Added new subcommand remove to commmad kraft pkg feat: Added new subcommand remove to command kraft pkg Jul 18, 2023
@nderjung nderjung changed the title feat: Added new subcommand remove to command kraft pkg feat: Added new subcommand kraft pkg rm Jul 19, 2023
@nderjung nderjung changed the title feat: Added new subcommand kraft pkg rm feat: Add new subcommand kraft pkg rm Jul 19, 2023
@craciunoiuc craciunoiuc added gsoc Google Summer of Code Contribution area/cmd Issue or PR related to all CLI programs labels Jul 20, 2023
@MdSahil-oss MdSahil-oss force-pushed the pkg-remove-to-update-yaml branch 2 times, most recently from a5d9ec9 to eb43e8d Compare July 25, 2023 10:04
@MdSahil-oss MdSahil-oss force-pushed the pkg-remove-to-update-yaml branch 3 times, most recently from 8f7d4e2 to 417d0d1 Compare August 7, 2023 15:22
@MdSahil-oss MdSahil-oss force-pushed the pkg-remove-to-update-yaml branch 2 times, most recently from 96ef9d1 to b0eaeb1 Compare August 15, 2023 19:25
@craciunoiuc craciunoiuc mentioned this pull request Aug 28, 2023
Merged
@nderjung nderjung changed the title feat: Add new subcommand kraft pkg rm feat: Introduce kraft pkg rm Aug 29, 2023
@craciunoiuc
Copy link
Member

For whatever reason, tests failed, it seems to be a compilation issue related to pkg remove and the Umbrella manager

@craciunoiuc craciunoiuc self-requested a review October 3, 2023 15:43
@MdSahil-oss MdSahil-oss force-pushed the pkg-remove-to-update-yaml branch 3 times, most recently from f27f7f6 to eeb3bcb Compare October 4, 2023 18:29
@nderjung nderjung closed this Oct 18, 2023
@nderjung nderjung reopened this Oct 18, 2023
@MdSahil-oss MdSahil-oss changed the title feat: Introduce kraft pkg rm feat: Introduce kraft lib rm Oct 29, 2023
@MdSahil-oss MdSahil-oss force-pushed the pkg-remove-to-update-yaml branch 2 times, most recently from db0c64f to 00eca2b Compare December 9, 2023 12:02
craciunoiuc
craciunoiuc requested changes Dec 11, 2023
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @MdSahil-oss

Some comments from my side.

Most important one is about preserving the order of the kraft yaml. I think that is mandatory for having this in.

Other than those comments, this works just fine and I'm ready to merge it once they are resolved

internal/cli/kraft/lib/remove/remove.go Outdated Show resolved Hide resolved
internal/cli/kraft/lib/remove/remove.go Outdated Show resolved Hide resolved
unikraft/app/application.go Outdated Show resolved Hide resolved
internal/cli/kraft/lib/remove/remove.go Outdated Show resolved Hide resolved
@MdSahil-oss
feat: Added new subcommand remove to commmad kraft lib
f52e9f0 
Signed-off-by: Md Sahil <mohdssahil1@gmail.com>
@craciunoiuc craciunoiuc self-assigned this Dec 17, 2023
craciunoiuc
craciunoiuc approved these changes Dec 17, 2023
View reviewed changes
Copy link
Member

@craciunoiuc craciunoiuc left a comment

Choose a reason for hiding this comment

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

All good here. Thanks!

Reviewed-by: Cezar Craciunoiu cezar.craciunoiu@unikraft.io
Approved-by: Cezar Craciunoiu cezar.craciunoiu@unikraft.io

@craciunoiuc craciunoiuc merged commit 4211c46 into unikraft:staging Dec 17, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@craciunoiuc craciunoiuc craciunoiuc approved these changes

@nderjung nderjung Awaiting requested review from nderjung

Assignees

@nderjung nderjung

@craciunoiuc craciunoiuc

Labels
area/cmd Issue or PR related to all CLI programs gsoc Google Summer of Code Contribution
Projects
KraftKit Roadmap
Status: 🚀 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MdSahil-oss @craciunoiuc @nderjung