Skip to content

Conversation

@amimimor
Copy link
Contributor

Description

  • Implement proposed sanitized names for topics, queues and subscriptions in SNS/SQS, instead of SHA256 hashed strings
  • Bugfix for deletion of SQS subscription on SNS upon daprd termination

Issue reference

#1034

#1029

Please reference the issue this PR will close: #[issue number]

Checklist

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

@amimimor amimimor requested review from a team as code owners July 24, 2021 13:03
@ghost
Copy link

ghost commented Jul 24, 2021

CLA assistant check
All CLA requirements met.

# This is the 1st commit message:

Improve error message in case of missing property (dapr#1012)

Co-authored-by: Artur Souza <[email protected]>
# This is the commit message dapr#2:

Remove vestigial pubsub/nats code (dapr#1024)

The pubsub/nats component was replaced by pubsub/natsstreaming as part
of dapr/dapr#2003, but the corresponding code
in dapr/components-contrib was not removed, so this change removes it.
# This is the commit message dapr#3:

bugfix for sns topic deletion upon termination

# This is the commit message dapr#4:

Revert "bugfix for sns topic deletion upon termination"

This reverts commit bcaa9bb.
@yaron2 yaron2 modified the milestone: v1.4 Jul 26, 2021
@yaron2
Copy link
Member

yaron2 commented Jul 26, 2021

/cc @pkedy

@daixiang0
Copy link
Member

lgtm

@amimimor
Copy link
Contributor Author

hi @daixiang0 @pkedy - sorry to bother, but can you trigger the build workflows so that this PR can proceed? I've run gofmt + gofumpt as the previous builds where crashing partly due to the lack of proper formatting (In my "defense" I'd say I didn't see those as required for contribution)

@amimimor
Copy link
Contributor Author

due to the passing of time my fork had drifted away from origin/master so I've merged it to be able to proceed with this PR. Awaiting approvers to trigger the build/test workflow

@artursouza artursouza self-assigned this Jul 30, 2021
@amimimor
Copy link
Contributor Author

@artursouza the build is failing because of Conformance Tests / state.redis
how do we proceed? (it's a dependency downloading issue)

@amimimor
Copy link
Contributor Author

amimimor commented Aug 4, 2021

hi @daixiang0 @pkedy @artursouza
I'd like to proceed with this PR but the build fails on other sub projects which I haven't changed, please advise
I'll be thankful if you could trigger another build and follow up on its results (as I'm suspecting it would fail)
Thanks

@yaron2
Copy link
Member

yaron2 commented Aug 6, 2021

hi @daixiang0 @pkedy @artursouza
I'd like to proceed with this PR but the build fails on other sub projects which I haven't changed, please advise
I'll be thankful if you could trigger another build and follow up on its results (as I'm suspecting it would fail)
Thanks

Sorry for the late response. running now.

@yaron2
Copy link
Member

yaron2 commented Aug 6, 2021

@amimimor were you able to test the component to make sure there are no regressions?

@amimimor
Copy link
Contributor Author

amimimor commented Aug 6, 2021 via email

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #1035 (3660b5b) into master (80f0110) will increase coverage by 0.49%.
The diff coverage is 51.13%.

❗ Current head 3660b5b differs from pull request most recent head d9d2316. Consider uploading reports for the commit d9d2316 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1035      +/-   ##
==========================================
+ Coverage   34.03%   34.53%   +0.49%     
==========================================
  Files         133      132       -1     
  Lines       10750    10870     +120     
==========================================
+ Hits         3659     3754      +95     
- Misses       6710     6736      +26     
+ Partials      381      380       -1     
Impacted Files Coverage Δ
pubsub/azure/eventhubs/eventhubs.go 23.91% <0.00%> (-4.30%) ⬇️
pubsub/azure/servicebus/servicebus.go 30.49% <0.00%> (-0.62%) ⬇️
pubsub/azure/servicebus/subscription.go 0.00% <0.00%> (ø)
pubsub/kafka/kafka.go 19.27% <0.00%> (+0.45%) ⬆️
state/mongodb/mongodb.go 16.76% <0.00%> (-0.20%) ⬇️
state/sqlserver/migration.go 0.00% <0.00%> (ø)
tests/conformance/common.go 14.93% <0.00%> (-0.42%) ⬇️
state/memcached/memcached.go 38.23% <50.00%> (+12.78%) ⬆️
state/cassandra/cassandra.go 25.97% <58.33%> (+2.73%) ⬆️
pubsub/aws/snssqs/snssqs.go 28.99% <60.00%> (+3.12%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38de305...d9d2316. Read the comment docs.

Copy link
Member

@yaron2 yaron2 left a comment

Choose a reason for hiding this comment

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

lgtm

@yaron2 yaron2 merged commit 253ef85 into dapr:master Aug 6, 2021
amimimor pushed a commit to amimimor/components-contrib that referenced this pull request Dec 9, 2021
…r#1035)

* bugfix for sns topic deletion upon termination

* Revert "bugfix for sns topic deletion upon termination"

This reverts commit bcaa9bb.

* wip on normalizing queue/topic names

* sanitize queue and topic names

* sanitized names. bugfix for close

* # This is a combination of 4 commits.
# This is the 1st commit message:

Improve error message in case of missing property (dapr#1012)

Co-authored-by: Artur Souza <[email protected]>
# This is the commit message dapr#2:

Remove vestigial pubsub/nats code (dapr#1024)

The pubsub/nats component was replaced by pubsub/natsstreaming as part
of dapr/dapr#2003, but the corresponding code
in dapr/components-contrib was not removed, so this change removes it.
# This is the commit message dapr#3:

bugfix for sns topic deletion upon termination

# This is the commit message dapr#4:

Revert "bugfix for sns topic deletion upon termination"

This reverts commit bcaa9bb.

* removed debug message

* raw string abort

* gofmt+remove regex and use byte iter

Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Amit Mor <[email protected]>
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.

5 participants