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

[EDGE-1520] Add pipeline templates rest & graphql api documentation #2479

Merged

Conversation

jonathanly
Copy link
Contributor

@jonathanly jonathanly commented Sep 18, 2023

Adds pipeline templates REST API and GraphQL documentation for the following PRs:

Note: graphql doc updates include bits around organization system banners, it's a side effect of generating the documentation via a schema file. I imagine this will resolve in the diff when #2486 is merged. Fixed conflicts via a rebase ✨

@buildkite-docs-bot
Copy link
Contributor

Preview URL: https://2479--bk-docs-preview.netlify.app

@jonathanly jonathanly changed the title [EDGE-1520] Add pipeline templates rest api documentation [EDGE-1520] Add pipeline templates rest & graphql api documentation Sep 22, 2023
Copy link
Contributor

@mhaylock mhaylock left a comment

Choose a reason for hiding this comment

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

@jonathanly I've not reviewed everything line by line, but I've reviewed the Preview URL and raised a few things that stand out to me.

PS: It seems that the diff did not clean up after #2486 was merged for some reason - maybe a rebase onto main to fix the merge conflicts will fix it?

pages/apis/rest_api/pipeline_templates.md Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
@jonathanly jonathanly force-pushed the edge-1520-create-pipeline-template-rest-api-documentation branch from 8be7a5a to 4e73e17 Compare September 29, 2023 03:52
@jonathanly jonathanly marked this pull request as ready for review September 29, 2023 03:55
@jonathanly jonathanly force-pushed the edge-1520-create-pipeline-template-rest-api-documentation branch 2 times, most recently from e9552ea to f7526fd Compare September 29, 2023 06:29
@jonathanly jonathanly force-pushed the edge-1520-create-pipeline-template-rest-api-documentation branch from da7cd63 to 7e0d3ed Compare October 2, 2023 23:11
@jonathanly jonathanly requested a review from mhaylock October 2, 2023 23:12
@jonathanly jonathanly force-pushed the edge-1520-create-pipeline-template-rest-api-documentation branch from 06da97e to f376798 Compare October 2, 2023 23:16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also sorted the index list in alphabetical order here

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave it to the docs team to comment on whether this re-ordering is desirable or not.

Copy link
Contributor

@mhaylock mhaylock left a comment

Choose a reason for hiding this comment

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

@jonathanly wording for pipeline templates looks good to me, but I think we should exclude all changes relating to build retention.

There is also one syntax error to clean up, and I think the re-ordering of the GraphQL API nav should be run by the docs team.

data/graphql/schema.graphql Outdated Show resolved Hide resolved
pages/apis/graphql/cookbooks/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave it to the docs team to comment on whether this re-ordering is desirable or not.

@jonathanly jonathanly requested a review from mhaylock October 3, 2023 05:49
Copy link
Contributor

@mhaylock mhaylock left a comment

Choose a reason for hiding this comment

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

Looks good to me! Again I'll leave it to the docs reviewers to make a call on the re-ordering, but the rest of the content looks a-ok to me.

@@ -15,7 +15,7 @@ curl "https://api.buildkite.com/v2/organizations/{org.slug}/agents"
"id": "0b461f65-e7be-4c80-888a-ef11d81fd971",
"graphql_id": "QWdlbnQtLS1mOTBhNzliNC01YjJlLTQzNzEtYjYxZS03OTA4ZDAyNmUyN2E=",
"url": "https://api.buildkite.com/v2/organizations/my-great-org/agents/my-agent",
"web_url": "https://buildkite.com/organizations/buildkite/my-great-org/agents/0b461f65-e7be-4c80-888a-ef11d81fd971",
"web_url": "https://buildkite.com/organizations/my-great-org/agents/0b461f65-e7be-4c80-888a-ef11d81fd971",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean up of these doubled-up org names! 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 🎉

Copy link
Contributor

@mbelton-buildkite mbelton-buildkite left a comment

Choose a reason for hiding this comment

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

I've left quite a few comments, but nothing major. 😊

pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
@@ -13,5 +13,6 @@ There are recipes for a range of different topics, including:
- [Jobs](/docs/apis/graphql/cookbooks/jobs)
- [Agents](/docs/apis/graphql/cookbooks/agents)
- [Clusters](/docs/apis/graphql/cookbooks/clusters)
- [Pipeline templates](/docs/apis/graphql/cookbooks/pipeline-templates)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's match the order used in the navigation:

- [Agents](/docs/apis/graphql/cookbooks/agents)
- [Builds](/docs/apis/graphql/cookbooks/builds)
- [Clusters](/docs/apis/graphql/cookbooks/clusters)
- [Jobs](/docs/apis/graphql/cookbooks/jobs)
- [Pipelines](/docs/apis/graphql/cookbooks/pipelines)
- [Pipeline templates](/docs/apis/graphql/cookbooks/pipeline-templates)
- [Organizations](/docs/apis/graphql/cookbooks/organizations)
- [Teams](/docs/apis/graphql/cookbooks/teams)

(I think I saw the start of a conversation somewhere about the ordering of these pages, but I can't find it now. There were reasons they weren't in alphabetic order, but I think it's fine to switch to that given how many pages we have now)

Copy link
Contributor Author

@jonathanly jonathanly Oct 4, 2023

Choose a reason for hiding this comment

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

Great pick up @mbelton-buildkite, I lost track of which pages are automatically generated vs which ones aren't 😅.
That convo I had with Chris mentioned whether or not we should alphabetically sort links under the Pipelines REST API:
image

Do you have opinions on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh okay. Yeah, for reference docs with that many entries, I think moving to order alphabetically is a good idea 😊

I think I have a feature request somewhere to generate index sections like that on Overview pages, so one day it hopefully won't be manual!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks your for input @mbelton-buildkite, I'll go ahead and order the list alphabetically :D

@@ -15,7 +15,7 @@ curl "https://api.buildkite.com/v2/organizations/{org.slug}/agents"
"id": "0b461f65-e7be-4c80-888a-ef11d81fd971",
"graphql_id": "QWdlbnQtLS1mOTBhNzliNC01YjJlLTQzNzEtYjYxZS03OTA4ZDAyNmUyN2E=",
"url": "https://api.buildkite.com/v2/organizations/my-great-org/agents/my-agent",
"web_url": "https://buildkite.com/organizations/buildkite/my-great-org/agents/0b461f65-e7be-4c80-888a-ef11d81fd971",
"web_url": "https://buildkite.com/organizations/my-great-org/agents/0b461f65-e7be-4c80-888a-ef11d81fd971",
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 🎉

pages/apis/graphql/cookbooks/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/graphql/cookbooks/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
pages/apis/rest_api/pipeline_templates.md Outdated Show resolved Hide resolved
</tr>
<tr>
<th><code>available</code></th>
<td>When set to <code>true</code>, non-admins can assign the pipeline template can assigned to pipelines.<br><em>Example:</em> <code>false</code></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this change didn't get applied quite right. Should be:

Suggested change
<td>When set to <code>true</code>, non-admins can assign the pipeline template can assigned to pipelines.<br><em>Example:</em> <code>false</code></td>
<td>When set to <code>true</code>, non-admins can assign the pipeline template to pipelines.<br><em>Example:</em> <code>false</code></td>

I think this mistake is made in a few places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully that last commit I just pushed up covered the rest of it 🤞

<tr><th><code>name</code></th><td>Name of the pipeline template</td></tr>
<tr><th><code>description</code></th><td>Description of the pipeline template</td></tr>
<tr><th><code>configuration</code></th><td>YAML step configuration for the pipeline template</td></tr>
<tr><th><code>available</code></th><td>When set to <code>true</code>, the pipeline template can assigned to pipelines by non-admins<br><em>Default:</em> <code>false</code></td></tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

One last instance:

Suggested change
<tr><th><code>available</code></th><td>When set to <code>true</code>, the pipeline template can assigned to pipelines by non-admins<br><em>Default:</em> <code>false</code></td></tr>
<tr><th><code>available</code></th><td>When set to <code>true</code>, non-admins can assign the pipeline template to pipelines<br><em>Default:</em> <code>false</code></td></tr>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a full text search on the code just to be sure.. 😆
That ought to be it!

Copy link
Contributor

@mbelton-buildkite mbelton-buildkite left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for all your work on this! 😊

@jonathanly jonathanly merged commit 88b0309 into main Oct 4, 2023
1 check passed
@jonathanly jonathanly deleted the edge-1520-create-pipeline-template-rest-api-documentation branch October 4, 2023 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants