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

fix: update source registry #537

Merged
merged 1 commit into from
Jun 29, 2023
Merged

fix: update source registry #537

merged 1 commit into from
Jun 29, 2023

Conversation

elrrrrrrr
Copy link
Member

the registryInfo in pkg fullManifest should be updated when the package is migrated to another registry.

  1. 🐞 when query from DB, dynamically add registry information.
  2. ♻️ when hit cache, updating metadata should already trigger cache modifications.

当包从属 registryId 发生变化时,包 manifest 内的信息也需要同步更新

  1. 🐞 从 db 读取元信息时,实时添加 registry 信息
  2. ♻️ 从缓存读取元信息时,发布或修改元信息时已触发缓存修改

@elrrrrrrr elrrrrrrr added the bug Something isn't working label Jun 29, 2023
}

const distBytes = Buffer.from(JSON.stringify(data));
const distIntegrity = await calculateIntegrity(distBytes);
etag = `"${distIntegrity.shasum}"`;

Choose a reason for hiding this comment

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

这段代码主要是在 PackageManagerService 类中的一个方法中进行了修改。我将逐步解释每个修改并提出改进建议:

  1. 在第一处修改中,原先的代码是判断 cmd.packageJson._source_registry_name 是否存在,如果不存在则获取 registry 并将其赋值给 _source_registry_name。现在的修改将获取 registry 的部分移到了条件判断之外,并且不再进行存在性的检查,而是直接将获取到的 registry.name 赋值给 cmd.packageJson._source_registry_name

    改进建议:这样的修改使得代码更加简洁和一致。条件判断是否存在 _source_registry_name 可以被移除,这样无论 _source_registry_name 是否存在都会进行赋值操作。在调用 this.getSourceRegistry(pkg) 之前,可以添加对 pkg 是否存在的检查来避免潜在的错误。

  2. 第二处修改与第一处类似,在满足条件的情况下,将 registry?.name 赋值给 data._source_registry_name

    改进建议:与第一处修改相同,可以移除对 _source_registry_name 存在性的检查,直接进行赋值操作。

总体而言,这些修改使代码更简洁、易读,并且保持了一致性。没有明显的错误风险,但仍然可以根据实际需求进一步优化和简化代码。


res = await app.httpRequest()
.get(`/${pkg.name}`);
assert.equal(res.body._source_registry_name, 'self');
});

it('should publish on user custom scopes', async () => {

Choose a reason for hiding this comment

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

对于给定的代码补丁,以下是一个简要的代码审查:

  1. 在修改之前:

    • 通过GET请求,在调用savePackage函数之后,断言res.body._source_registry_name等于'default'。
    • 删除了一些没有提供的相关代码,检查是否会对修改产生影响。
  2. 在修改之后:

    • 通过GET请求,在调用findPackage函数之后,断言res.body._source_registry_name等于'self'。
    • 删除了一些没有提供的相关代码,检查是否会对修改产生影响。

改进建议:

  • 添加更多详细的错误处理和异常情况处理逻辑。
  • 根据你的应用需求,可能需要进一步检查其他功能和边界情况的测试。

风险:

  • 没有提供完整的代码和上下文,因此无法全面评估潜在的风险。请确保修改不会破坏现有的功能,并且通过充分的测试覆盖率来验证修改的正确性。

请注意,这只是一个简要的代码审查意见,具体的问题和改进取决于代码的实际运行环境和需求。

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #537 (1f9381e) into master (dd69606) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
- Coverage   96.97%   96.97%   -0.01%     
==========================================
  Files         174      174              
  Lines       16591    16588       -3     
  Branches     2174     2173       -1     
==========================================
- Hits        16089    16086       -3     
  Misses        502      502              
Impacted Files Coverage Δ
app/core/service/PackageManagerService.ts 98.86% <100.00%> (-0.01%) ⬇️

@elrrrrrrr elrrrrrrr merged commit ab2fde7 into master Jun 29, 2023
13 checks passed
@elrrrrrrr elrrrrrrr deleted the update-source-registry branch June 29, 2023 07:49
fengmk2 pushed a commit that referenced this pull request Jun 29, 2023
[skip ci]

## [3.35.1](v3.35.0...v3.35.1) (2023-06-29)

### Bug Fixes

* update source registry ([#537](#537)) ([ab2fde7](ab2fde7))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants