Fix dist missing issue for npm package installing #151
Conversation
Pull Request Test Coverage Report for Build 2081376736
💛 - Coveralls |
ligangty
left a comment
ligangty
left a comment
There was a problem hiding this comment.
Something needs to change as comments. And I'd suggest to add a unit test to verify this case.
charon/constants.py
Outdated
| MANIFEST_SUFFIX = ".txt" | ||
| DEFAULT_ERRORS_LOG = "errors.log" | ||
|
|
||
| NRRC_REGISTRY = "npm.registry.redhat.com" |
There was a problem hiding this comment.
I think we should leave this out as a configuration item below each "target", as it is not same in different environments. We can set this as DEFAULT_NRRC_REGISTRY with a default value(for example "localhost" ) if it is not specified in configuration,
There was a problem hiding this comment.
@ligangty If for multi targets uploading, and configuring different registries with the targets, it has to handle the different metadata content-generating for different target/env, I want to confirm there will be any such case we will encounter in prod/stage?
There was a problem hiding this comment.
Yes there will be such case. We will not prevent user to upload one tarball to both prod/stage in one time.
charon/utils/archive.py
Outdated
|
|
||
|
|
||
| def __parse_npm_package_version_paths(path: str) -> list: | ||
| def _del_none(d): |
There was a problem hiding this comment.
I remember there is a same func in npm.py, if so, please remove it and reuse this one.
charon/utils/archive.py
Outdated
| shasum = digest(path, HashType.SHA1) | ||
| dist = dict() | ||
| dist["tarball"] = tarball_link | ||
| dist["shasum"] = shasum |
There was a problem hiding this comment.
There is also a "integrity" item in this dist section, which has "sha512-xxxxxx" value.
* Add registry in target bucket configuration * Different registries dist-tarball gen for multi-targets * Tarball integrity computing for dist support * Add unit testings for dist gen computing against s3
ligangty
left a comment
ligangty
left a comment
There was a problem hiding this comment.
Generally looks good except a small performance point.
charon/pkgs/npm.py
Outdated
| valid_dirs = __get_path_tree(valid_paths, target_dir) | ||
|
|
||
| # main_target = targets[0] | ||
| client = S3Client(aws_profile=aws_profile, dry_run=dry_run) |
There was a problem hiding this comment.
To save performance, we don't need to initiate this client multi times for different targets.
3 participants
No description provided.