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

Allow passing multiple --resources-path flags to daprd #5909

Merged
merged 10 commits into from Mar 8, 2023

Conversation

ItalyPaleAle
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle commented Feb 8, 2023

This is a spin-off from #5908 which takes one of the two parts of the PR (the largest one) as described in #5585

It allows starting daprd with multiple --resources-path flags, to specify multiple folders to load resources (components, resiliency policies, declarative subscriptions) from.

This allows, for example, maintaining components and resiliency policies in different folders, or maintain "shared" components in a separate folder from app-specific ones. More details can be found in #5585

It also includes some small tweaks and fixes encountered along the way.

TODO

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested review from a team as code owners February 8, 2023 16:21
Copy link
Member

@artursouza artursouza left a comment

Choose a reason for hiding this comment

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

Have not looked at the code yet, but I do miss E2E test for this. Is it possible with the current K8s centric approach? Is there a way to "manually" add the sidecar container in the test without relying on injector?

@ItalyPaleAle
Copy link
Contributor Author

ItalyPaleAle commented Feb 8, 2023

Because this only works in self-hosted mode, we cannot have E2E tests for this (same reason as every other time, sadly). It is also not meant to be run on K8s (that's what #5908 was for). Perhaps E2E tests could be created in the CLI repo when the CLI adds supports for this.

@yaron2
Copy link
Member

yaron2 commented Feb 8, 2023

Have not looked at the code yet, but I do miss E2E test for this. Is it possible with the current K8s centric approach? Is there a way to "manually" add the sidecar container in the test without relying on injector?

We don't have E2E for self-hosted mode today, although technically it's possible add them by configuring a self-hosted sidecar on a Kubernetes deployment.

it is also not meant to be run on K8s (that's what #5908 was for).

There are two separate issues here being mingled into one:

  1. Allow using --resources-path in Kubernetes mode #5908 tried to make a self-hosted feature apply to k8s mode. this is wrong and shouldn't be allowed. It included additional features that have been moved into this PR, which is great and unblocks this functionality, allowing it to be added in this PR.

  2. The changes in this PR are targeting self hosted mode, which we don't have E2E tests for (but would be great to design for those in the future as self-hosted is also a supported production mode of Dapr)

@ItalyPaleAle
Copy link
Contributor Author

although technically it's possible add them by configuring a self-hosted sidecar on a Kubernetes deployment.

Albeit possible, this is not easy at all given our K8s test framework today. The test runner doesn't have code in place to deploy a daprd container (converting annotations into CLI flags etc), and some things would require large efforts such as being able to retrieve the TLS certificates to inject them into the Deployment.

I agree with you both there's a need for self-hosted E2E tests, but I kindly ask that be considered outside of the scope of this PR given the large effort.

In the meanwhile, I think this PR can be "E2E tested" in the Dapr CLI with relative ease.

@yaron2
Copy link
Member

yaron2 commented Feb 8, 2023

I agree with you both there's a need for self-hosted E2E tests, but I kindly ask that be considered outside of the scope of this PR given the large effort.

For sure, as we don't have an infrastructure for self-hosted E2E tests, I don't expect this PR to be any different than any other self-hosted PR in terms of expectations.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #5909 (b657917) into master (e4cbccf) will decrease coverage by 0.14%.
The diff coverage is 49.49%.

❗ Current head b657917 differs from pull request most recent head 9f15eed. Consider uploading reports for the commit 9f15eed to get more accurate results

@@            Coverage Diff             @@
##           master    #5909      +/-   ##
==========================================
- Coverage   64.75%   64.62%   -0.14%     
==========================================
  Files         165      165              
  Lines       17249    17274      +25     
==========================================
- Hits        11170    11163       -7     
- Misses       5214     5237      +23     
- Partials      865      874       +9     
Impacted Files Coverage Δ
pkg/runtime/cli.go 1.50% <0.00%> (-0.08%) ⬇️
pkg/resiliency/resiliency.go 73.02% <38.46%> (+0.28%) ⬆️
pkg/runtime/runtime.go 67.16% <57.14%> (-0.39%) ⬇️
pkg/runtime/pubsub/subscriptions.go 72.91% <70.00%> (+0.45%) ⬆️
pkg/components/disk_manifest_loader.go 85.13% <94.44%> (+2.32%) ⬆️
pkg/components/local_loader.go 100.00% <100.00%> (ø)
pkg/runtime/config.go 84.61% <100.00%> (-0.39%) ⬇️
pkg/placement/membership.go 52.31% <0.00%> (-6.49%) ⬇️
pkg/placement/placement.go 81.72% <0.00%> (-2.16%) ⬇️
pkg/messaging/direct_messaging.go 57.74% <0.00%> (-0.84%) ⬇️
... and 2 more

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

@ItalyPaleAle ItalyPaleAle reopened this Mar 3, 2023
@artursouza artursouza merged commit 77136eb into dapr:master Mar 8, 2023
@artursouza artursouza added this to the v1.11 milestone Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants