Skip to content

Configuration API response to use dictionaries - based on PR #4695 #5083

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Aug 26, 2022
Merged

Configuration API response to use dictionaries - based on PR #4695 #5083

merged 9 commits into from
Aug 26, 2022

Conversation

berndverst
Copy link
Member

@berndverst berndverst commented Aug 25, 2022 •
edited
Loading

Description

This includes all commits from PR #4695 from @beiwei30 - thanks for the contributions! To speed up getting this feature merged I opened this new PR with all requires fixes / changes.

Fixes #4556
Fixes #4695
Fixes #5071

@berndverst berndverst requested review from a team as code owners August 25, 2022 20:42
beiwei30 and others added 8 commits August 25, 2022 13:58
@beiwei30 @berndverst
fix: Converting configuration API response from an array to dictionary.
15b6306 
dapr#4556

Signed-off-by: Ian Luo <ian.luo@gmail.com>
@beiwei30 @berndverst
use replace to pass compilation.
538fffc 
Signed-off-by: Ian Luo <ian.luo@gmail.com>
@beiwei30 @berndverst
fix compilation
b58e7ce 
Signed-off-by: Ian Luo <ian.luo@gmail.com>
@beiwei30 @berndverst
fix unit test
7e85305 
Signed-off-by: Ian Luo <ian.luo@gmail.com>
@mcandeia @berndverst
Update http API for config dicts
ee81241 
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@berndverst
gen proto
6949fd9 
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@berndverst
Fix http tests
28a9d94 
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@berndverst
import latest contrib
5d389ea 
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 25, 2022 •
edited
Loading

Codecov Report

Merging #5083 (6d377de) into master (29233c6) will decrease coverage by 0.02%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master    #5083      +/-   ##
==========================================
- Coverage   65.47%   65.45%   -0.03%     
==========================================
  Files         113      113              
  Lines       12723    12721       -2     
==========================================
- Hits         8331     8326       -5     
- Misses       3809     3811       +2     
- Partials      583      584       +1
Impacted Files Coverage Δ
pkg/http/api.go 79.53% <66.66%> (+0.01%) ⬆️
pkg/runtime/runtime.go 67.74% <80.00%> (ø)
pkg/grpc/api.go 72.85% <100.00%> (-0.09%) ⬇️
pkg/actors/internal/placement.go 87.56% <0.00%> (-1.56%) ⬇️

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

mukundansundar
mukundansundar reviewed Aug 25, 2022
View reviewed changes
Copy link
Contributor

@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. just one question ...

pkg/grpc/api.go
for _, v := range getResponse.Items {
cachedItems = append(cachedItems, &commonv1pb.ConfigurationItem{
Key: v.Key,
cachedItems := make(map[string]*commonv1pb.ConfigurationItem, len(getResponse.Items))
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for this to be a pointer to ConfigItem?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mukundansundar this is not my PR. This is from @beiwei30 - I don't know. I just dealt with the messy merge conflicts, missing tests and changes, and other broken things. If maintainers don't want it to be a pointer, please go ahead and fix up this PR here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dapr/maintainers-dapr any thoughts on avoiding pointers unless necessary?

@berndverst
Copy link
Member Author

@mukundansundar now this PR is failing because I rebased against latest dapr/dapr which introduces a change from @ItalyPaleAle..

we have about 5 conflicting changes with circular dependencies in flight. It's a complete mess.

@mukundansundar
Copy link
Contributor

@mukundansundar now this PR is failing because I rebased against latest dapr/dapr which introduces a change from @ItalyPaleAle..

we have about 5 conflicting changes with circular dependencies in flight. It's a complete mess.

wow ... all of them are failing! seems type mismatch ... probably use a prior commit from components-contrib to finish this PR and later a PR to use latest contrib ?

@berndverst
Copy link
Member Author

@mukundansundar I think I know how to fix.. but it's a mess. Stay tuned.

@berndverst berndverst marked this pull request as draft August 25, 2022 21:20
artursouza
artursouza previously approved these changes Aug 25, 2022
View reviewed changes
@artursouza artursouza self-requested a review August 25, 2022 21:35
@berndverst
make modtidy-all
6d377de 
Signed-off-by: Bernd Verst <4535280+berndverst@users.noreply.github.com>
@berndverst berndverst marked this pull request as ready for review August 26, 2022 01:29
@berndverst
Copy link
Member Author

Hopefully it's fixed now -- I had to get a change from @ItalyPaleAle into contrib and then import the latest contrib into this PR.

@berndverst berndverst requested review from mukundansundar and artursouza and removed request for artursouza and mukundansundar August 26, 2022 01:31
@berndverst berndverst mentioned this pull request Aug 26, 2022
Closed
@artursouza artursouza merged commit 683a436 into dapr:master Aug 26, 2022
@addjuarez addjuarez mentioned this pull request Aug 26, 2022
Closed
Taction added a commit to Taction/dapr that referenced this pull request Sep 5, 2022
@Taction
Merge branch 'master' into feat_add_cache_for_secret
5d216cc 
* master: (134 commits)
  Record metric for service updates (dapr#5114)
  chore: remove duplicate word in comments (dapr#5099)
  Run e2e tests in previous version of K8s on AKS to see if it's breaking Windows tests (dapr#5104)
  add {namespace} option to set consumerID
  Add tracing passthrough for bindings (dapr#5049)
  Improve component init error logs and metrics (dapr#4975)
  Improvements to E2E test apps (dapr#5042)
  Fix component name for aws dynamodb (dapr#5095)
  Fix perf tests not building (dapr#5096)
  Fix missing components in Metadata API result. (dapr#5052)
  Fixed log message (dapr#5088)
  Disable IPFS binding for now (dapr#5092)
  optimize/concurrency: pubsub concurrency map writes in runtime unit test. (dapr#5086)
  Configuration API response to use dictionaries - based on PR dapr#4695 (dapr#5083)
  correct compile error
  correct checking for actors with same id
  fix type error in print
  fix typo
  add log when actor start time and end time is not expected
  Components auto-registration (dapr#5062)
  ...

Signed-off-by: zhangchao <zchao9100@gmail.com>

# Conflicts:
#	go.mod
#	pkg/grpc/api.go
#	pkg/grpc/api_test.go
#	pkg/http/api.go
#	pkg/runtime/runtime.go
@berndverst berndverst mentioned this pull request Sep 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@mukundansundar mukundansundar mukundansundar left review comments

@artursouza artursouza artursouza approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Converting configuration API response from an array to dictionary.
5 participants
@berndverst @mukundansundar @artursouza @beiwei30 @mcandeia