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

Nexus #577

Merged
merged 11 commits into from
Jul 19, 2024
Merged

Nexus #577

merged 11 commits into from
Jul 19, 2024

Conversation

bergundy
Copy link
Member

@bergundy bergundy commented May 23, 2024

What was changed

  • Added Nexus Endpoint CRUD commands
  • Added Pending Nexus Operations and Callbacks to workflow describe command.

@antlai-temporal
Copy link
Contributor

The fix for tqname is already in the versioning-2 branch but it may take a while (blocked on go SDK merge) until we merge it. It is just two lines, and I don't want to block your PR, do you want to go ahead and add them?

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Mostly LGTM (waiting for server support before approving). Also, would like to see the tcld equivalent to ensure similarity.

Comment on lines +482 to +489
* `--description` (string) - Endpoint description in markdown format (encoded using the configured codec server).
* `--description-file` (string) - Endpoint description file in markdown format (encoded using the configured codec server).
Copy link
Member

Choose a reason for hiding this comment

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

This was removed in cloud API at temporalio/api-cloud#23 (comment), I think we should remove in this one too. Or we can have description, but I don't think we should get specific with the format expected at this time (I suspect there is a subset of markdown we will start using as part of temporalio/features#486).

Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to discuss this more. There's a strict requirement to have this description. We have it in all of the docs.
I've replied in that cloud API thread to request bringing the description back.

Copy link
Member

@cretz cretz May 23, 2024

Choose a reason for hiding this comment

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

Is there a strict requirement to have the description, or a strict requirement to say descriptions are markdown formatted? My issue is more with the latter and I think it should at least be subject to discussion.

Also, if this matched user metadata spec a bit better, there would be summary and details, not description. Workflows are going to get --summary and --details soon I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we want markdown. Maybe it's worth consolidating on summary and details to match workflow metadata, I would be okay changing that now.

Copy link
Member

@cretz cretz May 24, 2024

Choose a reason for hiding this comment

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

I am not sure we can generally say we want all long-form "description" text accepted by Temporal APIs to be full markdown, and whatever we do want Temporal long-form format to be should be centrally/generally documented. Setting a long-form format specific to Nexus descriptions and not for Temporal as a whole I don't think is the best approach.

Can you just assume Nexus descriptions have to be like every other long-form Temporal description/details and then we can discuss/decide/document that long-form in a general way that helps everyone and is not just for Nexus?

@bergundy bergundy closed this Jul 19, 2024
@bergundy bergundy reopened this Jul 19, 2024
@bergundy bergundy changed the base branch from main to nexus July 19, 2024 18:03
@bergundy bergundy merged commit 55aebb8 into temporalio:nexus Jul 19, 2024
19 checks passed
@bergundy bergundy deleted the nexus branch July 19, 2024 18:04
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.

4 participants