-
Notifications
You must be signed in to change notification settings - Fork 224
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
Remove references to cli/cmd-options.mdx #3122
base: main
Are you sure you want to change the base?
Conversation
@@ -1739,10 +1739,6 @@ | |||
"source": "/tctl-next/workflow#reset", | |||
"destination": "/cli/workflow#reset" | |||
}, | |||
{ | |||
"source": "/tctl-next/modifiers#--event-id", |
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.
@fairlydurable I'm not seeing any references to /tctl-next
outside of vercel.json, would it be appropriate to remove all of the tctl-next
references in this file?
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 not recommend messing with any tctl
redirects at this time. tctl
was our workhorse for years and many sources still link in.
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.
got it, any objection to pointing it to the broad /ci/index
page then? There doesn't seem to be a better replacement path for this that I can see
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.
Linking to /cli/cmd-options#event-id is semantically connected to what that inbound link is looking for. It gets them to the right revised place. I'm going to say it really depends on how we re-structure the page for clarity and concision. If there is a semantic equivalent, we should always do that. If that anchor goes away, then we can be less specific if it still gets them close to that information.
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.
Feedback from team: "Until we no longer support a feature or product, we’ll keep the documentation around and keep redirects in place as needed." No changes to redirects for now.
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.
Got it, since the anchor is going away, I think /cli
is the closest we can get to this information (although with this link not existing today, it's not clear what information it's trying to link anyways).
I'm also removing a duplicate set of redirects, it looks like the same source URLs occur twice in this file.
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.
That's good to know, but I still think /cli
is the right redirect here. There is no reference to "modifiers" in the docs, so there's no clear location to set a manual anchor. I think the answer here is to the CLI docs and let the user find what they're looking for.
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'm putting this as changes requested.
- The shortening the link text is optional
- I do believe that we do not want to remove tctl redirects, even if they aren't found on the site. I've ask in-team for clarity, but leaving this blocked until then.
Thank you for understanding.
What does this PR do?
Removes references to
cli/cmd-options.mdx
in preparation to remove the file itself.Notes to reviewers
See temporalio/cli#685 for more context on WIP CLI doc efforts