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

Jump to bottom

Add Custom Search Attributes to TemporalNamespace #567

Open
dmytro-orlenko opened this issue Dec 5, 2023 · 10 comments · May be fixed by #776
Open

Add Custom Search Attributes to TemporalNamespace #567

dmytro-orlenko opened this issue Dec 5, 2023 · 10 comments · May be fixed by #776
Labels
improvement

Comments

@dmytro-orlenko
Copy link
Contributor

Custom Search Attributes as a part of namespace and can help to search workflow by attributes

apiVersion: temporal.io/v1beta1
kind: TemporalNamespace
metadata:
  name: default
spec:
  clusterRef:
    name: temporal
  description: Default namespace
  retentionPeriod: 336h #14 days
  allowDeletion: false
  customSearchAttributes:
    key: value
@sandhyabakash
Copy link

+1 This would be a very useful feature.

@alexandrevilain
Copy link
Owner

Hi @dmytro-orlenko !

Good idea! Are you interested on contributing for this feature?

@vinimona
Copy link

@alexandrevilain Really appreciate your efforts in addressing the issues!! I think it would be really convenient to be able to add custom search attributes via the TemporalNamespace manifest. Thanks.

@alexandrevilain alexandrevilain added this to the 0.17.0 milestone Jan 4, 2024
@dmytro-orlenko
Copy link
Contributor Author

dmytro-orlenko commented Jan 9, 2024 •
edited
Loading

@alexandrevilain I'm not sure about my changes, can you help me with it? #602

@alexandrevilain alexandrevilain mentioned this issue Jan 9, 2024
Closed
@alexandrevilain
Copy link
Owner

Hi @dmytro-orlenko !

Thanks for your contribution.
Looks like you took the wrong direction, RegisterNamespaceRequest and UpdateNamespaceRequest does not provide the CustomSearchAttributes field. See: https://pkg.go.dev/go.temporal.io/api/workflowservice/v1#RegisterNamespaceRequest

According to the temporal documentation about search attributes, the command to use is: temporal operator search-attribute create --name CustomSA --type Keyword --namespace yournamespace.

After a quick look at the cli's codebase, we can find the code handling this in the cli, see: https://github.com/temporalio/cli/blob/main/searchattribute/search_attribute_commands.go#L62

Looks like the search attribute reconciliation should be done in another step, after the namespace has been created :)

Hope it helps.

@dmytro-orlenko
Copy link
Contributor Author

It looks more complicated than I thought

@alexandrevilain
Copy link
Owner

I can help you implementing this @dmytro-orlenko :)

@alexandrevilain alexandrevilain modified the milestones: 0.17.0, 0.18.0 Feb 24, 2024
@alexandrevilain alexandrevilain removed this from the 0.18.0 milestone Apr 29, 2024
@TheHiddenLayer
Copy link
Contributor

fyi @alexandrevilain, i'm planning to start work on this in the near term

@TheHiddenLayer
Copy link
Contributor

TheHiddenLayer commented Jul 19, 2024 •
edited
Loading

@alexandrevilain apologies for the delay. I've implemented custom search attributes in my fork.

Based on my light manual testing, looks like it works 😄. The only thing that's making me hesitate to make the PR is some more testing. Will do more manual testing tomorrow, and will probably need to write some code test in tests/e2e/namespace_test.go.

wdyt?

@alexandrevilain
Copy link
Owner

alexandrevilain commented Jul 22, 2024 •
edited
Loading

Cool @TheHiddenLayer ! Thanks for your work!

Feel free to open a Draft pull request ;)
For sure unit and e2e tests are always welcomed and encouraged ;)

@TheHiddenLayer TheHiddenLayer linked a pull request Jul 23, 2024 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
5 participants
@alexandrevilain @vinimona @TheHiddenLayer @sandhyabakash @dmytro-orlenko