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

Rename components directory to resources under .dapr directory #1149

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

pravinpushkar
Copy link
Contributor

@pravinpushkar pravinpushkar commented Dec 9, 2022

Description

  • this pr changes dapr init behaviour to create resources dir instead of components and then creates a symlink components for resources directory.
  • if there is any pre-existing components dir, dapr init will also moves its contents into resources dir, deletes the components directory and then creates the symlink for resources directory.
  • dapr run --resources-path -> uses provided path in through flag
  • dapr run --components-path -> uses provided path in through flag
  • dapr run -> precedence order is resources dir then components dir.
  • since components and resources will always be in sync, it wont matter where the new resources are put but the recommendation is resources directory as components will be deprecated in future.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1148

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

@pravinpushkar pravinpushkar changed the title Feat/resources dir Rename components directory to resources under .dapr directory Dec 9, 2022
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #1149 (d605587) into master (211e57b) will increase coverage by 0.46%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
+ Coverage   26.11%   26.58%   +0.46%     
==========================================
  Files          38       38              
  Lines        2604     2656      +52     
==========================================
+ Hits          680      706      +26     
- Misses       1858     1878      +20     
- Partials       66       72       +6     
Impacted Files Coverage Δ
pkg/standalone/standalone.go 5.08% <20.68%> (+0.78%) ⬆️
pkg/standalone/common.go 58.92% <55.55%> (-2.98%) ⬇️

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

Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
@pravinpushkar pravinpushkar marked this pull request as ready for review December 12, 2022 12:44
@pravinpushkar pravinpushkar requested review from a team as code owners December 12, 2022 12:44
Signed-off-by: Pravin Pushkar <[email protected]>
cmd/run.go Outdated Show resolved Hide resolved
pkg/standalone/standalone.go Outdated Show resolved Hide resolved
pkg/standalone/standalone.go Outdated Show resolved Hide resolved
pkg/standalone/standalone.go Outdated Show resolved Hide resolved
tests/e2e/standalone/utils.go Outdated Show resolved Hide resolved
pkg/standalone/common.go Outdated Show resolved Hide resolved
pkg/standalone/common.go Outdated Show resolved Hide resolved
pkg/standalone/common.go Outdated Show resolved Hide resolved
pkg/standalone/standalone.go Outdated Show resolved Hide resolved
pkg/standalone/common.go Outdated Show resolved Hide resolved
pkg/standalone/standalone.go Outdated Show resolved Hide resolved
pravinpushkar and others added 3 commits December 13, 2022 12:07
Co-authored-by: Shubham Sharma <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Co-authored-by: Shubham Sharma <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
README.md Outdated Show resolved Hide resolved
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
Signed-off-by: Pravin Pushkar <[email protected]>
shubham1172
shubham1172 previously approved these changes Jan 2, 2023
Signed-off-by: Pravin Pushkar <[email protected]>
if !isSlimMode() && daprContainerRuntime != "" && !utils.Contains(uninstallArgs, "--container-runtime") {
uninstallArgs = append(uninstallArgs, "--container-runtime", daprContainerRuntime)
}
return spawn.Command(common.GetDaprPath(), uninstallArgs...)
Copy link
Member

Choose a reason for hiding this comment

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

Why not reuse cmdUninstall, similar to installDapr function above?

Comment on lines -132 to -161
daprContainerRuntime := containerRuntime()

// Add --container-runtime flag only if daprContainerRuntime is not empty, or overridden via args.
// This is only valid for non-slim mode.
if !isSlimMode() && daprContainerRuntime != "" && !utils.Contains(args, "--container-runtime") {
uninstallArgs = append(uninstallArgs, "--container-runtime", daprContainerRuntime)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove this instead of moving it to cmdUninstall and using this instead of the new function in utils?

@dapr-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@mukundansundar
Copy link
Collaborator

@pravinpushkar Please fix conflicts in this PR.

@msfussell @yaron2 I think this PR can go in 1.11. (Renaming default components dir to resources dir) Thoughts?

@pravinpushkar pravinpushkar marked this pull request as draft February 28, 2023 17:23
Signed-off-by: Pravin Pushkar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename components directory to resources under .dapr directory
7 participants