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

Implement the tool registry in the piped side #5343

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

Warashi
Copy link
Contributor

@Warashi Warashi commented Nov 18, 2024

What this PR does:

as title

Why we need it:

We decided the installation tools should be done on the piped side, not the plugin. ref; #5218

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?: No

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 48.64865% with 57 lines in your changes missing coverage. Please review.

Project coverage is 25.38%. Comparing base (23eddac) to head (f9f7bf0).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go 65.06% 21 Missing and 8 partials ⚠️
pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go 0.00% 22 Missing ⚠️
pkg/app/pipedv1/cmd/piped/piped.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5343      +/-   ##
==========================================
+ Coverage   25.29%   25.38%   +0.09%     
==========================================
  Files         444      445       +1     
  Lines       47656    47772     +116     
==========================================
+ Hits        12053    12126      +73     
- Misses      34661    34696      +35     
- Partials      942      950       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Comment on lines 30 to 34
type toolRegistry struct {
toolsDir string
tmpDir string
group singleflight.Group
}
Copy link
Member

Choose a reason for hiding this comment

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

Since the tool is not plugin-specific, I think we should have some kind of cache to avoid redundant installation. If the name and version are the same, then we should return the installed path immediately instead of running the script, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I'll add the cache like below.

r.mu.RLock()
_, ok := r.versions[name]
r.mu.RUnlock()
if ok {
return path, false, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@khanhtc1202
fixed on this commit, thanks
75d7daa

Comment on lines 48 to 49
func newToolRegistry(toolsDir, tmpDir string) (*toolRegistry, error) {
tmpDir, err := os.MkdirTemp(tmpDir, "tool-registry")
Copy link
Member

Choose a reason for hiding this comment

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

[Ask] Is it enough just to create the tmp dir for the tool installation in the newToolRegistry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to call like os.MkdirTemp("", "tool-registry") and do not pass the tmpDir as the argument?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean fixing as newToolRegistry(toolsDir string) and call os.MkdirTemp inside it as you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got your point! That's enough.
I'll change the codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed on this commit
f9f7bf0

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Good to go 👍

@Warashi Warashi merged commit 7213dcf into master Nov 19, 2024
18 checks passed
@Warashi Warashi deleted the pipedv1-piped-side-toolregistry branch November 19, 2024 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants