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

Feature/add tenant id to job / create process command #683

Conversation

torstenzuther
Copy link
Contributor

This PR adds tenant ID to

  • create process command
  • job worker builder

closes parts of #591

@CLAassistant
Copy link

CLAassistant commented May 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution @torstenzuther I appreciate your time and effort.

In general looks good there is one thing we have to clarify about the code, see below.

Furthermore, please take a look at our contribution guide related to the commit guidelines, please adjust them.

For example: use feat: add tenantId to create process command

@@ -64,6 +65,18 @@ public IJobWorkerBuilderStep3 Handler(AsyncJobHandler handler)
return this;
}

public IJobWorkerBuilderStep3 TenantIds(IList<string> tenantIds)
{
Request.TenantIds.Clear();
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 Do we really want to clear the list?

This behaves different from the rest of the API where we can chain multiple calls. My expectation would be that it just adds together.

Example:

command.TenantIds(12, 34).TenantIds(56) -> results in {12, 34, 56) and not (56)

Copy link
Contributor Author

@torstenzuther torstenzuther May 24, 2024

Choose a reason for hiding this comment

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

Hi @Zelldon

thanks for the review.
Yes that makes sense. I changed it so that it is aligned with the rest of the API.

Commit msg changed as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you !

@torstenzuther torstenzuther force-pushed the feature/add-tenant-id-to-job branch from bcd5986 to fee6be4 Compare May 24, 2024 08:13
@ChrisKujawa ChrisKujawa merged commit a8ee193 into camunda-community-hub:main May 24, 2024
5 checks passed
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.

3 participants