Skip to content

support resources-path #1012

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 20 commits into from
Dec 8, 2022

Conversation

hueifeng
Copy link
Member

Description

Signed-off-by: HueiFeng 695979933@qq.com

Issue reference

#953

Please reference the issue this PR will close: #953

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Signed-off-by: HueiFeng <695979933@qq.com>
@hueifeng hueifeng requested review from a team as code owners June 27, 2022 16:31
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #1012 (e21d42e) into master (63f14c3) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
- Coverage   29.18%   29.16%   -0.03%     
==========================================
  Files          35       35              
  Lines        2364     2366       +2     
==========================================
  Hits          690      690              
- Misses       1598     1599       +1     
- Partials       76       77       +1     
Impacted Files Coverage Δ
pkg/standalone/run.go 59.06% <0.00%> (-0.70%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@hueifeng Based on the decision in the issue #953 , docs needs to be added and changes be made.
Also for the existing code please add E2E tests here.

Signed-off-by: hueifeng <695979933@qq.com>
Signed-off-by: hueifeng <695979933@qq.com>
@hueifeng hueifeng force-pushed the feature/spec-resources-path branch from 62259d2 to 4a0888a Compare August 11, 2022 11:16
@hueifeng hueifeng changed the title support spec-resources-path support resources-path Aug 11, 2022
Signed-off-by: hueifeng <695979933@qq.com>
@hueifeng hueifeng mentioned this pull request Aug 11, 2022
7 tasks
@msfussell
Copy link
Member

msfussell commented Aug 29, 2022

There needs to be some clarity on next steps before this PR is merged

  1. Does this PR create a local resources directory as there is for components? I believe this is needed, since this is where you can put your subscriptions and resiliency policies. See below.

| --components-path, -d | | Linux & Mac: $HOME/.dapr/components, Windows: %USERPROFILE%\.dapr\components | The path for components directory |
| --resources-path | | | The path for resources directory |

  1. Can I also put Components into this directory and they are loaded by the runtime?
  2. Does this affect Configuration YAML files? Are these treated as resources also?
  3. Are we going to deprecate the --components-path? If so then this needs a tracking issue as part of this PRs next steps
  4. Are we going to update thedapr initto put anything into thes resources directory?
  5. The docs need more updates than just the CLI. This is an important change and needs to be covered in other docs. I have left comments into the docs issue.

@addjuarez
Copy link

@hueifeng

@hueifeng
Copy link
Member Author

hueifeng commented Sep 7, 2022

Currently we are using substitution in CLI, by default using --components-path, by way of judgement in daprd binary. I want to reduce unnecessary code and replace --components-path directly with resources-path after the next two releases.

  1. Can I also put Components into this directory and they are loaded by the runtime?

    Now we need to specify it with the --resources-path parameter.

  2. Are we going to deprecate the --components-path? If so then this needs a tracking issue as part of this PRs next steps

    According to our initial plan, and the current PR is a direct remove.

Maybe I still need to do a lot of work and maybe we can discuss what's next and whether to make big changes to the PR. What do you guys have in mind?

cc @msfussell @yaron2 @mukundansundar @pravinpushkar

Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@hueifeng Can you add an E2E for the resources path flag?
Also add usage and in readme.

pravinpushkar and others added 3 commits December 7, 2022 15:54
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>
Signed-off-by: Pravin Pushkar <ppushkar@microsoft.com>
Copy link
Collaborator

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

lgtm

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation required This issue needs documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename the run --components-path flag to a name for suitable for loading many resource types
7 participants