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

[skip changelog]Fix issue in ResourceManager and nopfsPlugin about repo path #10492

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fengzie
Copy link

@fengzie fengzie commented Aug 24, 2024

For ResourceManager and nopfsPlugin, we need pass the Repo path for use, instead of getting from config.PathRoot(), which might not be the right one.

This will also fixes issue #10407 .

@fengzie fengzie requested a review from a team as a code owner August 24, 2024 03:19
@lidel lidel mentioned this pull request Aug 28, 2024
31 tasks
@lidel
Copy link
Member

lidel commented Sep 10, 2024

Triage notes:

  • we want to fix denylist support first, we plan to look into it in 0.31 (Release 0.31 #10499), then we will revisit this PR
    • understand why we need it, and was not a problem before

@lidel lidel added the status/blocked Unable to be worked further until needs are met label Sep 10, 2024
@lidel
Copy link
Member

lidel commented Sep 23, 2024

@fengzie just to be sure this is triage properly, mind elaborating what you mean by "we need pass the Repo path for use, instead of getting from config.PathRoot(), which might not be the right one."?

What would be example of state or use case where config.PathRoot() is "not be the right one"?

@lidel lidel added the need/author-input Needs input from the original author label Sep 23, 2024
Comment on lines +26 to +27
// Path is the repo file-system path
Path() string
Copy link
Member

@lidel lidel Sep 23, 2024

Choose a reason for hiding this comment

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

What about in-memory or remote / virtual repos?

my understanding is that reading path from config makes more sense, because we know that the config file in filesystem somewhere, and resourcemanager or nopfs files could live there.

Copy link
Author

Choose a reason for hiding this comment

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

Currently only FSRepo is provided. For the other kind of repo implementations, there should be design to be compatible with resourcemanager or nopfs.

func (r *FSRepo) Path() string {

@gammazero gammazero added P1 High: Likely tackled by core team if no one steps up and removed need/author-input Needs input from the original author status/blocked Unable to be worked further until needs are met labels Sep 24, 2024
@gammazero gammazero self-assigned this Sep 24, 2024
@fengzie
Copy link
Author

fengzie commented Sep 25, 2024

@fengzie just to be sure this is triage properly, mind elaborating what you mean by "we need pass the Repo path for use, instead of getting from config.PathRoot(), which might not be the right one."?

What would be example of state or use case where config.PathRoot() is "not be the right one"?

Hi @lidel , for config.PathRoot(), it gets the value from environment variable firstly, if empty it uses the default DefaultPathRoot. However, when creating an IPFS instance, user can specify the repo path instead of the default one.

If user create multiple instances through the SDK, then there might be issue.

func PathRoot() (string, error) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants