Hacktoberfest 2026: những issue maintainer đã đánh dấu cho tháng Mười, đang mở và phù hợp người mới. Xem issue Hacktoberfest

refactor(config): deduplicate exec helpers between secret-store, writer and config-resolver

Đã đóng
  • Có gì không ổn?
#739 1 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức phù hợp với người mới
20/100
Loại issue
Tái cấu trúc
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
typescript
Lĩnh vực
cli

Hướng nghiên cứu

Start by diffing src/config/secret-store.ts, src/config/writer.ts and packages/config-resolver/src/index.ts to confirm the duplicated helpers (shellEscape, psEncodedCommand, PRINTABLE_ASCII_RE, execOpts, the _testSetExecSync/_testSetPlatform seams). The issue says not to start until #664 and the icacls-to-execFile change for writer.ts have landed, so re-check those first. Done means no helper body is defined in more than one file, existing tests pass with only import paths changed, and the lazy load in loader.ts is preserved.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

Stale

Summary

Process-exec helpers are copy-pasted across three files. Diffed and confirmed byte-identical:

  • shellEscape and psEncodedCommand: src/config/secret-store.ts:88-95 and packages/config-resolver/src/index.ts:139-146.
  • PRINTABLE_ASCII_RE: secret-store.ts:57 and config-resolver/src/index.ts:184.
  • execOpts: config-resolver/src/index.ts:135-137 and src/config/writer.ts:59-61 are identical. secret-store.ts:77-86 is the same with an optional input.
  • _execSync / _platform plus the _testSetExecSync / _testSetPlatform seams: secret-store.ts:125-148, config-resolver/src/index.ts:132-133,341-351, writer.ts:42-57.

secret-store.ts also defines spawnOpts (L110-119). Its body is identical to execOpts (L77-86); only the return type differs. The security -w … doc comment at L101-106 describes the spawnSync(SETSID_PERL, …) call in MacKeychainStore.put (~L184-195), but it is attached to the SETSID_PERL constant.

Sequencing (do NOT start yet)

  1. After #664's fix. This comment proposes moving secret-store.ts to execFile/spawn argv. That deletes every POSIX shellEscape use in that file (L214, 239, 252, 283, 294) and probably execOpts/spawnOpts as they are today.
  2. After the icacls → execFile PR for writer.ts. writer.ts only uses _execSync for icacls (L238). Once that change lands, its execOpts copy and _testSetExecSync seam either go away or change shape.

What remains after both

These are estimates and depend on how #664 lands:

  • psEncodedCommand (4 lines) is still in both secret-store.ts (Windows Credential Manager: L313, 330, 342) and config-resolver (L297).
  • PRINTABLE_ASCII_RE (1 line) is in both.
  • shellEscape stays only in config-resolver (L215, 238, 264) unless the resolver also moves to argv. It would then no longer be duplicated.
  • The _testSetExecSync / _testSetPlatform seams (~10 lines) remain in each file that still shells out.

That leaves roughly 15-25 duplicated lines in total. This is low priority. Once the prerequisites land, re-run the diff first. If only psEncodedCommand and the regex are left, it may be cheaper to leave them and close this issue.

Proposal

  • Export psEncodedCommand (and PRINTABLE_ASCII_RE, plus the exec options builder if one survives) from @elastic/config-resolver, which is already a workspace dependency, and import them in secret-store.ts. Don't add a src/lib/proc.ts: the package can't import from src/, so the package has to be the source.
  • Keep it a cheap import. loader.ts:31,381 lazy-loads @elastic/config-resolver to keep the help path fast. secret-store.ts is only reached from config commands and cloud/credentials.ts, so a static import there should be fine. Check this against ARCHITECTURE.md before merging.
  • Collapse execOpts/spawnOpts into one builder if both still exist.
  • Move the security -w comment onto the spawnSync call it describes.

Acceptance criteria

  • No helper function body is defined in more than one of src/config/secret-store.ts, src/config/writer.ts, packages/config-resolver/src/index.ts.
  • At most one exec options builder in secret-store.ts.
  • The security -w comment sits next to the spawnSync call in MacKeychainStore.put.
  • No behavior change. Existing secret-store, writer and config-resolver tests pass unmodified, apart from import paths.
  • No regression in help-path startup time (the lazy-load in loader.ts is preserved).

Related

#664, plus the pending icacls → execFile PR for src/config/writer.ts.

authored by Pi Coding Agent (Claude Opus 5.5)

Ngôn ngữ chính
TypeScript
Star
51
Fork
27
Merge trung bình
22 giờ 48 phút
Pull request đã merge (30 ngày)
48

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của elastic/cli

Tất cả issue của elastic/cli

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.