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

Sparse checkout - kcl mod add --package #453

Merged
merged 29 commits into from
Aug 20, 2024

Conversation

officialasishkumar
Copy link
Contributor

@officialasishkumar officialasishkumar commented Aug 18, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

This PR adds a functionality that allows to add a single package in the dependency incase the repo contains multiple of them. It does this as follows:

  1. kcl mod add --git <url> --commit <hash> --package <package_name> (done)

After running this command, KPM will download the whole repo (also works if there is no kcl.mod in the root) and specifies package in the kcl.mod file. It changes the full_name, name and other similar variables in kcl.mod.lock and kcl.mod as per the package name (it doesn't uses the repo name if package flag is there).

  1. kcl mod run (done)

This command will first checkout the directory which contains the package by running a recursive function. It will then do all the usual kcl compilation based on that directory.

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar
Copy link
Contributor Author

officialasishkumar commented Aug 18, 2024

PTAL @zong-zhe @Peefy

This PR is different from the last one since here I am using the package functionality in add subcommand. I have written a description which explains more about this PR. I hope it helps!

Also, Please let me know if there are any other tasks that I am missing out on the description that needs to be done.

I will update the kcl mod run command soon.
kcl mod run is working fine now

@coveralls
Copy link

coveralls commented Aug 18, 2024

Pull Request Test Coverage Report for Build 10465292101

Details

  • 48 of 91 (52.75%) changed or added relevant lines in 6 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.3%) to 40.642%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/package/modfile.go 11 14 78.57%
pkg/downloader/source.go 0 5 0.0%
pkg/client/client.go 10 16 62.5%
pkg/downloader/toml.go 0 7 0.0%
pkg/utils/utils.go 27 36 75.0%
pkg/cmd/cmd_add.go 0 13 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/package/modfile.go 1 40.87%
pkg/downloader/toml.go 2 0.0%
Totals Coverage Status
Change from base Build 10458285505: 0.3%
Covered Lines: 3177
Relevant Lines: 7817

💛 - Coveralls

@zong-zhe
Copy link
Contributor

Hi @officialasishkumar 😄

There is a null pointer exception in the e2e test, you may need to check these test cases and fix the null pointer.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar
Copy link
Contributor Author

Hi @zong-zhe ,

Thank you for the comment. Will fix it soon!

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar officialasishkumar force-pushed the sparse-checkout-kcl-mod-add branch from 699f3eb to 56b3ff8 Compare August 18, 2024 15:19
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar
Copy link
Contributor Author

@zong-zhe fixed the failing tests

@Peefy
Copy link
Contributor

Peefy commented Aug 19, 2024

cc @Zongzhe

@@ -52,6 +52,8 @@ type KpmClient struct {
settings settings.Settings
// The flag of whether to check the checksum of the package and update kcl.mod.lock.
noSumCheck bool
// The package to use in case of multiple packages.
pkg string
Copy link
Contributor

Choose a reason for hiding this comment

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

It may not be appropriate to add this field to KpmClient, which is the client of the package management tool, and it should not be associated with a specific package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment @zong-zhe. I have removed it!

@zong-zhe
Copy link
Contributor

Hi @officialasishkumar 😄

Good job. The overall direction of your work is correct, and most of it has been completed. Now, there are still a few problems. In addition to the comments I gave you in the code, you also need to add some test cases for the functions you developed to ensure that your functions are normal. It mainly includes: 1. To test whether kcl.mod with package field can work well. 2. Test whether you can download a subpackage from a github repository.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar officialasishkumar force-pushed the sparse-checkout-kcl-mod-add branch from 6842206 to 64621be Compare August 19, 2024 13:06
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar
Copy link
Contributor Author

Hey @zong-zhe,

I have addressed the comments. Please have a look!

@zong-zhe
Copy link
Contributor

Hey @zong-zhe,

I have addressed the comments. Please have a look!

Hi @officialasishkumar 😄

Add a test case to test the dependency is added as expected to kcl.mod and kcl.mod.lock, prepare the expected.mod and expected.mod.lock, empty kcl.mod and kcl.mod.lock, after adding the dependency, compare the kcl.mod/kcl.mod.lock with expected.mod/expected.mod.lock.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar officialasishkumar force-pushed the sparse-checkout-kcl-mod-add branch from 37a11e7 to fee67ab Compare August 20, 2024 04:07
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@officialasishkumar
Copy link
Contributor Author

Hi @zong-zhe

i have added the mentioned test case

Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

Good Job! LGTM

@Peefy Peefy merged commit 45c9fb3 into kcl-lang:main Aug 20, 2024
6 checks passed
@officialasishkumar
Copy link
Contributor Author

@Peefy Are there any other deliverables left for LFX?

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.

Enhancement: kpm add command displaying optimization
4 participants