-
Notifications
You must be signed in to change notification settings - Fork 44
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
Enhancement: Supports adding third-party dependencies from git repo with the version field #430
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Manoramsharma <[email protected]>
[dependencies] | ||
[dependencies.flask-demo-kcl-manifests] | ||
name = "flask-demo-kcl-manifests" | ||
full_name = "flask_manifests_0.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the main reason for your test failure, and it is also where the PR is not yet complete, currently full_name
is used to generate the path of the local dependency. So full_name
should be <name>_<git_tag>
, <name>_<git_commit>
or <name>_<git_branch>
, Instead of <name>_<version>
, the problem was caused by the old local dependency file storage structure, and finish #384 work was more helpful in solving the problem.
I suggest stopping this PR for a while to concentrate on #384
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GenDepFullName will generate the full name of a dependency by its name and version
// based on the '<package_name>_<package_tag>' format.
func (dep *Dependency) GenDepFullName() string {
dep.FullName = fmt.Sprintf(PKG_NAME_PATTERN, dep.Name, dep.Version)
return dep.FullName
}
Can this issue be resolved by changing the function mentioned above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got your point now @zong-zhe
The CI failed. cc @Manoramsharma |
@zong-zhe told me to stop this PR until the #384 will be completed. |
I see. Thank you! |
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
fix : #266
2. What is the scope of this PR (e.g. component or file name):
pkg/client/client.go
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
I simply changed the logic of AddDepToPkg function to handle version field at the time of adding the dependencies to the pkg.
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
I directly modified the test_data for testing the unit tests. No need to make any other changes in unit test.