-
Notifications
You must be signed in to change notification settings - Fork 217
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
Handle Nexus links #1605
Handle Nexus links #1605
Conversation
71b00c5
to
ebcfdd8
Compare
3828bbe
to
2709aa9
Compare
2709aa9
to
7803b3b
Compare
7803b3b
to
ae9bcfe
Compare
converter/link_converter.go
Outdated
rePatternNamespace = fmt.Sprintf(`(?P<%s>[a-z0-9-.]+)`, urlPathNamespaceKey) | ||
rePatternWorkflowID = fmt.Sprintf(`(?P<%s>[^/]+)`, urlPathWorkflowIDKey) | ||
rePatternRunID = fmt.Sprintf(`(?P<%s>[a-zA-Z0-9-]+)`, urlPathRunIDKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be defining these regexps here, you should be able to split the paths and let the server do the validation.
If this made it into the server codebase, you should change there too, I may have missed it Friday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, on the server, namespace doesn't have this character limitation:
func (wh *WorkflowHandler) validateNamespace(
namespace string,
) error {
if err := common.ValidateUTF8String("Namespace", namespace); err != nil {
return err
}
if len(namespace) > wh.config.MaxIDLengthLimit() {
return errNamespaceTooLong
}
return nil
}
If we changed the validation logic, we wouldn't want the SDK and server to go out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add anything on the server side. I did here in SDK only.
converter/link_converter.go
Outdated
return nil, fmt.Errorf("failed to parse link to Link_WorkflowEvent") | ||
} | ||
|
||
matches := urlPathRE.FindStringSubmatch(link.URL.EscapedPath()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just use splits by /
here but this is fine if the regexp doesn't limit characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought that validating the individual parts of the split a bit ugly. Regex looks cleaner.
converter/link_converter.go
Outdated
)) | ||
) | ||
|
||
func ConvertLinkWorkflowEventToNexusLink(we *commonpb.Link_WorkflowEvent) nexus.Link { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is all of this intended to be public user-facing API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need these functions in the server.
converter/link_converter.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this converter
package is for data/failure conversion, not for any thing that might need to be converted. I think this Nexus-specific stuff should go in the temporalnexus
package. And I am not sure we should expose these utilities to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point. There's a want to reuse this code in the server codebase to avoid duplicating this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Can move it and leave it public and document it as user facing since it will be
We should expose the links in the workflow API on the We can do that in a follow up PR AFAIC. |
6ee5334
to
000358a
Compare
}, | ||
Type: "temporal.api.common.v1.Link.WorkflowEvent", | ||
}, | ||
outputURL: "temporal:///namespaces/ns/workflows/wf-id/run-id/history?eventID=1&eventType=WorkflowExecutionStarted&referenceType=EventReference", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of the run ID being a query param too, but meh not important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the UI it's in the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much of this is meant to match UI path vs HTTP API paths?
}, | ||
Type: "temporal.api.common.v1.Link.WorkflowEvent", | ||
}, | ||
outputURL: "temporal:///namespaces/ns/workflows/wf-id/run-id/history?eventID=1&eventType=WorkflowExecutionStarted&referenceType=EventReference", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the UI it's in the path.
f4ab96e
to
8952557
Compare
8952557
to
89148d1
Compare
What was changed
Handle Nexus links
Why?
Support Nexus bi-directional links.
Checklist
Closes
How was this tested: