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 checker module to validate kpm three-party dependencies #470

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

NishantBansal2003
Copy link
Contributor

@NishantBansal2003 NishantBansal2003 commented Aug 28, 2024

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

re #394

  • N
  • Y

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

kcl-lang/kpm/pkg/checker

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

This PR contains an isolated implementation of the checker module to validate KPM third-party dependencies. The changes have not yet been integrated with KPM third-party dependencies; integration will be done in the future once the checker module design is finalised. The PR includes:

  • IdentChecker, which validates the dependency names in kclPkg.
  • VersionChecker, which validates the dependency versions in kclPkg.
  • SumChecker, which validates the dependency checksums in kclPkg.
  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

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

There are some breaking changes: calling the SumChecker function will not work properly as the implementation of getTrustedSum is incomplete and requires further discussion and design before completion. Once finalized, the SumChecker can be called without causing any breaking issues.

  • N
  • Y

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

Currently, unit tests for the checker module design have not been completed. These will be completed in the future upon further discussion and completion of this PR.
To verify the changes in the pkg/checker/ directory run the following commands:

go test ./... -coverprofile=coverage.out
go tool cover -html=coverage.out -o coverage.html
open coverage.html
  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@NishantBansal2003
Copy link
Contributor Author

cc: @zong-zhe

@zong-zhe
Copy link
Contributor

Hi @NishantBansal2003 😄

You need to add more test cases to Checker.

@coveralls
Copy link

coveralls commented Aug 28, 2024

Pull Request Test Coverage Report for Build 10601682765

Details

  • 48 of 50 (96.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 41.075%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/checker/checker.go 48 50 96.0%
Totals Coverage Status
Change from base Build 10589856639: 0.3%
Covered Lines: 3318
Relevant Lines: 8078

💛 - Coveralls

@NishantBansal2003
Copy link
Contributor Author

You need to add more test cases to Checker.

Do you mean I need to add unit tests for the existing functions of the Checker module? Since SumChecker is incomplete, I can add unit tests for IdentChecker and VersionChecker.

@NishantBansal2003
Copy link
Contributor Author

I have added unit tests. Please take a look.
cc: @zong-zhe

@Peefy Peefy merged commit 75d8fcb into kcl-lang:main Aug 29, 2024
6 checks passed
@NishantBansal2003
Copy link
Contributor Author

Hey @zong-zhe, regarding the implementation of getTrustedSum, should we proceed with the discussion, or is there anything else we need to address first?

@zong-zhe
Copy link
Contributor

Hey @zong-zhe, regarding the implementation of getTrustedSum, should we proceed with the discussion, or is there anything else we need to address first?

Hi @NishantBansal2003 😄

You can continue to add internal implementations to this structure to support its ability to fetch the sum of dependencies.

type SumChecker struct{}

@NishantBansal2003
Copy link
Contributor Author

Hey @zong-zhe, regarding the implementation of getTrustedSum, should we proceed with the discussion, or is there anything else we need to address first?

Hi @NishantBansal2003 😄

You can continue to add internal implementations to this structure to support its ability to fetch the sum of dependencies.

type SumChecker struct{}

Sure working on it...

@NishantBansal2003
Copy link
Contributor Author

You can continue to add internal implementations to this structure to support its ability to fetch the sum of dependencies.

Hey @zong-zhe, could you elaborate on this a bit more so I can research it in the meantime and propose a solution? Also, will there be any meetings scheduled in the future regarding the project?
Thanks!

@zong-zhe
Copy link
Contributor

zong-zhe commented Sep 2, 2024

Hi @NishantBansal2003 😄

kpm only needs to complete the sum check of OCI dependency at present, and there are some incomplete implementations of this part in kpm at present, kpm fetch sum from OCI registry from here:

func (c *KpmClient) AcquireDepSum(dep pkg.Dependency) (string, error) {

You need to transfer the unfinished work above to SumChecker to complete, and how this part of the code structure can be better organized, you may need to refer to the helm or golang implementation in this part.

@NishantBansal2003
Copy link
Contributor Author

Hi @zong-zhe,
The above code can be transferred, but how should I run the unit tests for these changes? Since the current KCL dependency does not include org.kcllang.package.sum in the manifest, my plan is to run a local registry using Docker. I will first push a test package that includes org.kcllang.package.sum, then pull it and verify its checksum.
Could you please advise if this approach is appropriate?

@zong-zhe
Copy link
Contributor

zong-zhe commented Sep 3, 2024

approach

Hi @NishantBansal2003 😄

Yes, this is the right direction, and the kpm project currently supports this, you do not need to do any other work, just run this script to get a local OCI Registry at localhost:5001, the account name is test and password is 1234.

https://github.com/kcl-lang/kpm/blob/main/scripts/reg.sh

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