Closed
Bug 1222982
Opened 9 years ago
Closed 9 years ago
API docs should show API stability
Categories
(Taskcluster :: Services, defect)
Taskcluster
Services
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: pmoore, Assigned: kraldav4)
References
Details
(Whiteboard: [good first bug][mentor=pmoore])
Attachments
(2 files, 3 obsolete files)
For example, in the meta data, cancelTask is "experimental", but there is no visual indication of this in the API docs: http://docs.taskcluster.net/queue/api-docs/#cancelTask Similarly, importClients is "deprecated" in meta data, but this can not be seen from API docs: http://docs.taskcluster.net/auth/api-docs/#importClients It might be also be nice to set a background shade for the API function name in the function list at the top of each API page, e.g. grey for deprecated, and maybe something else for experimental. A similar visual cue could be given in the body of the function documentation too, as well as explicitly stating "deprecated" or "experimental".
Updated•9 years ago
|
Component: Tools → Documentation
Comment 1•9 years ago
|
||
Maybe we should just get rid of API stability -- I'm not sure it's useful information..
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jopsen)
Reporter | ||
Comment 2•9 years ago
|
||
I kind of like it, as long as we maintain it. I think if we start showing it, we are more likely to maintain it.
Comment 3•9 years ago
|
||
Just like all of the "more later" docs have gotten filled in, right? :)
Comment 4•9 years ago
|
||
e.g., https://github.com/taskcluster/azure-entities/blob/master/README.md
Reporter | ||
Comment 5•9 years ago
|
||
I see your point. Jonas was the driving force behind stability levels - let's see what he says. It was added less than six months ago in https://github.com/taskcluster/taskcluster-base/pull/12. The stability levels are displayed in the go docs of the go client, e.g. see https://godoc.org/github.com/taskcluster/taskcluster-client-go/queue#Queue.CancelTask - I'm not sure if anything else currently displays them (I suspect not). @Jonas, I'd say if you'd like to keep it, please go through the existing APIs and set appropriate stability levels, e.g. it looks like all the Queue APIs are "experimental", which doesn't make sense now we're in production. Or we say the module owner is responsible for updating the stability levels. If you agree this is fair, we can create bugs for each module, otherwise let's drop it, as dustin suggests.
Comment 6•9 years ago
|
||
@pmoore, dustin, We do have API that becomes deprecated and APIs that never goes further than experimental.. aws-provisioner APIs are largely just experimental, as we someday may wish to change the concept dramatically. I like it. and put adding stability on my todo. There is quiet a few components to go through which is probably why I never got around to it.
Flags: needinfo?(jopsen)
Reporter | ||
Comment 7•9 years ago
|
||
I think this could be a good starter bug for someone. Note - the dependent bugs are about setting correct values for stability - this bug itself is about *showing* stability in the docs.
Whiteboard: [good first bug][mentor=pmoore]
Assignee | ||
Comment 8•9 years ago
|
||
Hello, I would like to try this as my first bug, can you give me some info please? :)
Comment 9•9 years ago
|
||
Hi David -- I'll get you started and Pete can pick up later. We have a bunch of services that use taskcluster-lib-api (https://github.com/taskcluster/taskcluster-lib-api) to provide REST API endpoints. One such service is taskcluster-hooks (https://github.com/taskcluster/taskcluster-hooks), but there are many more. In declaring an API endpoint, the service can specify a stability level, for example https://github.com/taskcluster/taskcluster-hooks/blob/master/routes/v1.js#L159 api.declare({ method: 'get', route: '/hooks/:hookGroupId/:hookId/schedule', name: 'getHookSchedule', output: 'hook-schedule.json', title: 'Get hook schedule', stability: 'deprecated', description: [ "This endpoint will return the schedule and next scheduled creation time", "for the given hook." ].join('\n') }, .. Taskcluster-lib-api takes all of this information and packages it into a JSON file which it publishes on https://references.taskcluster.net; for hooks the file is http://references.taskcluster.net/hooks/v1/api.json The documentation site, http://docs.taskcluster.net, contains JavaScript code which, in the browser, loads those JSON files and displays them to the user. For hooks, the page is http://docs.taskcluster.net/services/hooks/. You'll notice that that documentation page does not mention API stability levels, even though they are included in the JSON file. That's what you need to fix.
Comment 10•9 years ago
|
||
The documentation is in the https://github.com/taskcluster/taskcluster-docs repo. You may want to look specifically at https://raw.githubusercontent.com/taskcluster/taskcluster-docs/master/services/secrets/index.md https://github.com/taskcluster/taskcluster-docs/tree/master/assets/docref
Assignee | ||
Comment 11•9 years ago
|
||
OK, thanks very much, I'll try to look at it.
Updated•9 years ago
|
Assignee: nobody → kraldav4
Assignee | ||
Comment 12•9 years ago
|
||
Hello, can somebody give me an advice how to run the pages locally? I cloned the repository, but the default.html seems to be written in Django template lang that i have never seen before. Thanks :)
Updated•9 years ago
|
Flags: needinfo?(pmoore)
Reporter | ||
Comment 13•9 years ago
|
||
David, see https://github.com/taskcluster/taskcluster-docs#development-setup
Flags: needinfo?(pmoore)
Reporter | ||
Comment 14•9 years ago
|
||
@dustin, thanks for the detailed overview
Assignee | ||
Comment 15•9 years ago
|
||
Ok, now its running :) I added: <dt>Stability</dt> <dd><code><%= entry.stability%></code></dd> Do I need to add some styles depending on the value of stability? Or test if stability exists?
Flags: needinfo?(pmoore)
Reporter | ||
Comment 16•9 years ago
|
||
Hi David, thanks for your work! (In reply to David Král from comment #15) > Ok, now its running :) I added: <dt>Stability</dt> > <dd><code><%= entry.stability%></code></dd> > > Do I need to add some styles depending on the value of stability? That would be awesome, yes please! See comment 0 for some ideas how we could exploit this, feel free to play around with styles to see what you think looks best. > Or test if stability exists? From http://schemas.taskcluster.net/base/v1/api-reference.json it looks like stability should be always present, and that currently its value should be one of: * "deprecated" * "experimental" * "stable" However it would be best if the UI can gracefully handle other values being shown, or if no value is present, as e.g. this schema could change, and e.g. updating the docs site can easily be overlooked. Or maybe some API manages to creep in which somehow doesn't comply with the schema. Just a safeguard really, so that we don't suddenly end up with a broken docs site because something was overlooked. Thanks again for your help!
Flags: needinfo?(pmoore)
Assignee | ||
Comment 17•9 years ago
|
||
Stability added in reference.ejs
Attachment #8731326 -
Flags: review?
Assignee | ||
Comment 18•9 years ago
|
||
I created some styles for all three stability values. I warn that I am not very good in graphic design :))
Comment 19•9 years ago
|
||
Comment on attachment 8731326 [details] [diff] [review] diff file for reference.ejs Review of attachment 8731326 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure what the best approach is for styling these results -- I think there may be some bootstrap-y way to do this, rather than writing raw color values. Perhaps Jonas knows more about that? Otherwise, this looks good. Just some formal issues to fix. Please include both this change and the new css file in the same patch. Also, note that you have lines with spaces at the end of the line -- Bugzilla highlights these in red. Please remove such "trailing whitespace" from your patch. ::: assets/docref/reference.ejs @@ +48,5 @@ > var signature = entry.name + "(" + args.join(', ') + ") : " + retval; > + var stabilityValue = "unknown"; > + if (entry.stability) { > + stabilityValue = entry.stability; > + } This stanza can be shortened to var stabilityValue = entry.stability || "unknown";
Attachment #8731326 -
Flags: review?
Comment 20•9 years ago
|
||
Actually, maybe Pete knows how best to handle the styles.
Flags: needinfo?(pmoore)
Assignee | ||
Comment 21•9 years ago
|
||
Reporter | ||
Comment 22•9 years ago
|
||
Thanks for the patch David. The trailing space still needs to be removed from assets/docref/stability.css. When I deploy locally, I don't see different styles for the different stability levels, I'm not sure why not. Can you confirm you have different styles when you run it? Thanks, Pete
Flags: needinfo?(pmoore)
Assignee | ||
Comment 23•9 years ago
|
||
Because I dont know where to link css file. Should it be in default.html or somewhere else?
Flags: needinfo?(pmoore)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jopsen)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(pmoore)
Comment 24•9 years ago
|
||
@david, Yeah, why not add it here: https://github.com/taskcluster/taskcluster-docs/blob/de259ee88801f82a9a4a760a8d7d6fc7878ebc60/_layouts/default.html#L114 It's not as if we could make this file any more ugly than it already is :) Wow, I had completely forgotten how bad that code is... Anyways, for now better docs is better. - We can clean up this mess some rainy day (/me lives in SF, it doesn't rain there -- hi hi)
Flags: needinfo?(jopsen)
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8731326 -
Attachment is obsolete: true
Attachment #8731327 -
Attachment is obsolete: true
Attachment #8731414 -
Attachment is obsolete: true
Attachment #8734294 -
Flags: review?
Reporter | ||
Comment 26•9 years ago
|
||
Comment on attachment 8734294 [details] [diff] [review] patch.diff Review of attachment 8734294 [details] [diff] [review]: ----------------------------------------------------------------- I like this, thanks for working on this David! I'd also like to add a couple of extra changes, so I'll r+ this first, and then move the further changes into a PR. Your changes will land when that PR lands. Thanks again!
Attachment #8734294 -
Flags: review? → review+
Reporter | ||
Comment 27•9 years ago
|
||
Attachment #8737217 -
Flags: review?(dustin)
Comment 28•9 years ago
|
||
Commits pushed to master at https://github.com/taskcluster/taskcluster-docs https://github.com/taskcluster/taskcluster-docs/commit/b1973e8508d0193e3fa02d832319b96be2eaa02a Bug 1222982 - Add stability levels to API docs https://github.com/taskcluster/taskcluster-docs/commit/cb960e73804551449fd753a257d35ac8f6ac6ad7 Merge pull request #88 from taskcluster/stability Bug 1222982 - API docs should show API stability
Reporter | ||
Comment 29•9 years ago
|
||
Comment on attachment 8737217 [details] [review] Github Pull Request for taskcluster-docs Carrying over r+ from dustin from PR comments.
Attachment #8737217 -
Flags: review?(dustin) → review+
Reporter | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 30•9 years ago
|
||
Ok, thanks for your help too :)
Updated•6 years ago
|
Component: Documentation → Services
You need to log in
before you can comment on or make changes to this bug.
Description
•