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

PubSub API: standardize err msgs #7322

Merged
merged 60 commits into from Jan 16, 2024
Merged

Conversation

cicoyle
Copy link
Contributor

@cicoyle cicoyle commented Dec 19, 2023

Description

Making this a draft since I need to add integration tests.

This is the code to standardize many of the PubSub api errors to use the newly merged kit pkg for standardizing errors around the richer error model based on this proposal: https://github.com/dapr/proposals/blob/main/0009-BCIRS-error-handling-codes.md.

I added todos for a follow up PR to enrich more of the pubsub errors, as there were already so many that were updated in this PR.

Issue Reference

#6068

Checklist

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation: Elena has a PR for it: Error codes docs#3908
  • Provided sample for the feature / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

@@ -302,7 +303,7 @@ func TestPubSubEndpoints(t *testing.T) {
// assert
assert.Equal(t, 403, resp.StatusCode, "unexpected success publishing with %s", method)
assert.Equal(t, "ERR_PUBSUB_FORBIDDEN", resp.ErrorBody["errorCode"])
assert.Equal(t, "topic topic is not allowed for app id test", resp.ErrorBody["message"]) //nolint:dupword
assert.Equal(t, "topic topic is not allowed for app id fakeAPI", resp.ErrorBody["message"]) //nolint:dupword
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be fakeAPI, as if you look that is the app id that is set for the test. test is the pubsub name I believe, so I made it the true app id name.

Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
…ion tests

Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
@sadath-12
Copy link
Contributor

@cicoyle had a thought like instead of keeping under pkg/api we could keep it in pkg/utils something like that so we don't make the packages confusing between pkg/api and pkg/apis

…ubOutbox test w/ pluggable pubsub

Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
go.mod Outdated Show resolved Hide resolved
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
Signed-off-by: Cassandra Coyle <cassie@diagrid.io>
@artursouza
Copy link
Member

Does any of the current reviewers have any concern before I merge this?

@mukundansundar
Copy link
Contributor

/test-sdk-all

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jan 16, 2024

Dapr SDK Python test

🔗 Link to Action run

Commit ref: d68b1d8

✅ Python SDK tests passed

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jan 16, 2024

Dapr SDK Java test

🔗 Link to Action run

Commit ref: d68b1d8

❌ Java SDK tests failed

Please check the logs for details on the error.

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jan 16, 2024

Dapr SDK Go test

🔗 Link to Action run

Commit ref: d68b1d8

✅ Go SDK tests passed

@dapr-bot
Copy link
Collaborator

dapr-bot commented Jan 16, 2024

Dapr SDK JS test

🔗 Link to Action run

Commit ref: d68b1d8

✅ JS SDK tests passed

@cicoyle
Copy link
Contributor Author

cicoyle commented Jan 16, 2024

@cicoyle you can "fix" the failing windows tests by adding this check to your tests:

if runtime.GOOS == "windows" {
		t.Skip("skipping unix socket based test on windows")
	}

Check out Josh's PR for details: https://github.com/dapr/dapr/pull/7377/files#diff-63a8642d7d0d77c4c1c98a0b9a603522277d8bef886479027e0d7030d9cdfa47R57

If you look at the commit right before your comment, that's where I added the 'fix'. Its the actor health that is failing for me on Windows. I assume that is somewhat flaky bc I've seen it before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autoupdate DaprBot will keep the Pull Request up to date with master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants