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

feat: upsert search attributes #275

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

cv65kr
Copy link
Contributor

@cv65kr cv65kr commented Sep 28, 2022

Reason for This PR

temporalio/sdk-php#76

Same functionality like in GO-SDK - https://github.com/temporalio/samples-go/tree/main/searchattributes

Changes needed in PHP-SDK - temporalio/sdk-php#248

Description of Changes

Added missing implementation of UpsertWorkflowSearchAttributes command.

E2E

Changes: roadrunner-server/rr-e2e-tests#138

Workflow file: https://github.com/temporalio/sdk-php/blob/6a40e81ba7125ee32fbb242599f65ddff649500a/tests/Fixtures/src/Workflow/UpsertSearchAttributesWorkflow.php

Also definition of search attributes needs to be executed to test server before running workflow - https://github.com/temporalio/sdk-php/blob/6a40e81ba7125ee32fbb242599f65ddff649500a/tests/SearchAttributeTestInvoker.php

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

@rustatian rustatian changed the title Upsert search attributes feat: upsert search attributes Sep 28, 2022
@rustatian rustatian self-requested a review September 28, 2022 16:54
@rustatian rustatian added A-other Area: other C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. labels Sep 28, 2022
@rustatian rustatian added this to the v1.6.x milestone Sep 28, 2022
@rustatian
Copy link
Collaborator

Hey @cv65kr 👋🏻
Please, do not edit the PR template.

internal/protocol.go Outdated Show resolved Hide resolved
@cv65kr cv65kr force-pushed the upsert-search-attributes branch from aab4c14 to a0ce155 Compare September 28, 2022 17:11
@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 28, 2022

@rustatian Updated, also signed my commits. And big sorry for removing template 😄

I will back to you as soon as I test whole implementation together with SDK-PHP.

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 28, 2022

@rustatian fixed and tested. I did small mistake and encoder was not needed.

image

@cv65kr cv65kr marked this pull request as ready for review September 28, 2022 17:36
@rustatian
Copy link
Collaborator

@cv65kr Could you please add a PHP worker to the description (which sends this command)? I'll add it to our international tests: https://github.com/roadrunner-server/rr-e2e-tests.

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 28, 2022

@rustatian You meant workflow definition? I see general worker https://github.com/roadrunner-server/rr-e2e-tests/blob/master/php_test_files/temporal-worker.php

Also added info about definition of search attributes which need to be executed before running such test.

@cv65kr
Copy link
Contributor Author

cv65kr commented Sep 28, 2022

@rustatian to make your live easier I prepared changes in E2E repository - roadrunner-server/rr-e2e-tests#138 Feel free to modify it since I put my branches there, because we need to wait for merge.

@rustatian
Copy link
Collaborator

Great, thank you, @cv65kr 👍🏻
I'll review and merge it tomorrow ⚡

Copy link
Collaborator

@rustatian rustatian left a comment

Choose a reason for hiding this comment

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

lgtm

@rustatian rustatian merged commit 114f416 into temporalio:master Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-other Area: other C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants