Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adds Nexus Endpoint #23
Adds Nexus Endpoint #23
Changes from 24 commits
c2976e4
d7c380d
9081063
8e0dda3
e2f3ed8
d80870a
0c99cbb
bdf4a13
c7ac7e6
b1af03b
dfa7aee
53bd255
78eb26b
3a75ed7
7453e13
9760590
f7be5c4
060abc0
26c429c
5cb4b6b
80bc416
baa4151
4bb0de1
4d7f4da
2f131e2
b281878
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 you can just call this
id
, that's what I did in the operator 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.
so I changed this to id, but this is now in-consistent with our other APIs related to user and namespace operations (where the request/path takes in user_id or namespace_id). am fine with having this as id, but will check with @anekkanti to be sure
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 need to be consistent with the rest of the API here, that is more important than consistency with the self-hosted operator service
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.
changed it back
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.
Do we have guidance anywhere explaining why you'd want to specify this? I understand the concept but I'm not sure users do.
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.
Agree that this probably needs to be fleshed out more here: https://docs.temporal.io/ops. The API itself is still in preview. I have seen other APIs have a section about "idempotency" which describes the usage/semantics around similar fields. We may want something similar her.
cc @anekkanti @jlacefie in case there is some other documentation I am missing here, or to potentially track ensuring docs cover this eventually
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.
@rachfop since he is working on docs
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.
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 it time to add a formatter?
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 that can be discussed on the primary
api
repo and can apply to this if/when it moves thereThere 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.
Lets move to using an enum here.
@nikki-dag
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 should reuse this: https://github.com/temporalio/api-cloud/pull/36/files#diff-b38afecfa7fefa137fb5dbe446fd43bd6603676748e6f061fcf1f03d2e386706R13
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.
@mastermanu can we also show the identity that created and modified this endpoint?
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 we can, although we'd probably want to consider adding that across all the objects we have (e.g. users, namespaces, etc).
would probably consider adding that as a separate effort.