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.

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

update dapr runtime to use v1.12.0-rc.1 #906

Merged
merged 7 commits into from
Sep 25, 2023
Merged

update dapr runtime to use v1.12.0-rc.1 #906

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8099f80
update dapr runtime to use v1.12.0-rc.1
cicoyle Sep 14, 2023
0b41f72
update to latest rc for runtime and cli
cicoyle Sep 20, 2023
60accf9
try sleeping
cicoyle Sep 21, 2023
c2011ef
pin redis version
cicoyle Sep 21, 2023
9171319
rm sleep
cicoyle Sep 21, 2023
d66f11c
rm deprecated unsubscribe call
cicoyle Sep 22, 2023
64d67ec
Update configuration/go/sdk/order-processor/app.go
cicoyle Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/env/global.env
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
DAPR_CLI_VERSION: 1.11.0
DAPR_RUNTIME_VERSION: 1.11.0
DAPR_CLI_VERSION: 1.12.0-rc.1
DAPR_RUNTIME_VERSION: 1.12.0-rc.3
DAPR_INSTALL_URL: https://raw.githubusercontent.com/dapr/cli/v${DAPR_CLI_VERSION}/install/
DAPR_DEFAULT_IMAGE_REGISTRY: ghcr
MACOS_PYTHON_VERSION: 3.10
2 changes: 1 addition & 1 deletion .github/workflows/validate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ jobs:
run: |
helm repo add bitnami https://charts.bitnami.com/bitnami
helm repo update
helm install redis bitnami/redis
helm install redis bitnami/redis --version 17.14.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why do we want to pin? this can catch us later on too when we forget :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per Arturs comment above ^, tho also not sure on the exact reasoning for it

dapr init -k --runtime-version=${{ env.DAPR_RUNTIME_VERSION }} --wait || kubectl get pods --all-namespaces
kubectl get nodes -o wide
for pod in `dapr status -k | awk '/dapr/ {print $1}'`; do kubectl describe pod -l app=$pod -n dapr-system ; kubectl logs -l app=$pod -n dapr-system; done
Expand Down
1 change: 0 additions & 1 deletion configuration/go/sdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ name: Run order-processor service
expected_stdout_lines:
- '== APP == Configuration for orderId2: {"Value":"102","Version":"","Metadata":null}'
- '== APP == App subscribed to config changes with subscription id:'
- '== APP == App unsubscribed to config changes'
- "Exited App successfully"
expected_stderr_lines:
output_match_mode: substring
Expand Down
8 changes: 1 addition & 7 deletions configuration/go/sdk/order-processor/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,7 @@ func main() {
}
fmt.Println("App subscribed to config changes with subscription id: " + subscriptionID)

time.Sleep(10 * time.Second)
time.Sleep(5 * time.Second)
cicoyle marked this conversation as resolved.
Show resolved Hide resolved

err = client.UnsubscribeConfigurationItems(context.Background(), DAPR_CONFIGURATION_STORE, subscriptionID)
if err != nil {
fmt.Println("Error unsubscribing to config updates, err:" + err.Error())
} else {
fmt.Println("App unsubscribed to config changes")
}
Comment on lines -51 to -56
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this being removed?
Is this removed from SDK or not a valid operation anymore?

Copy link
Contributor

Choose a reason for hiding this comment

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

It has been deprecated in runtime https://github.com/dapr/dapr/blob/9105124636459e751964f3a68845fc63ccbb45e9/pkg/grpc/api.go#L1459

It should be marked as such in SDKs (I don't think it is atm anywhere).

Copy link
Contributor

@mukundansundar mukundansundar Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it should be removed ... but is it causing an issue ...

On a side note, @dapr/maintainers-quickstarts When will the latest SDK RCs will be tested for quickstarts.

Copy link
Contributor

@JoshVanL JoshVanL Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it should be removed ... but is it causing an issue ...

Yes, with it included the test runner is printing out an error saying that the subscription cannot be unsubscribed because it doesn't exist. That is a true error- the subscription at the top of the file is given a context with a timeout of 10 seconds, we then do some work, then sleep 10 seconds. This means that when we go to unsubscribe here, the subscription context has been cancelled and closed, and therefore unsubscribed. The unsubscribe here now makes no sense.

The relevant runtime PR: dapr/dapr#6769

Copy link
Contributor

Choose a reason for hiding this comment

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

ok that is marked as a breaking change ....

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the other question, is the unsubscribe method only failing in GO SDK?
Does it fail in any other SDK?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mukundansundar I assume it is not failing in other quickstart tests because the logic is different. For example, JS is doing the right thing here by just closing the stream

quickstarts/configuration/javascript/sdk/order-processor/index.js

Line 24 in e278ba4

try {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened a PR in the go-sdk repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the context timeout was also causing an issue due to us having a context with a timeout, then sleeping for that duration.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

os.Exit(0)
}
Loading