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

Upstreamize API guide #3212

Merged
merged 50 commits into from
Sep 6, 2024
Merged

Conversation

Lennonka
Copy link
Contributor

@Lennonka Lennonka commented Aug 12, 2024

Offering the API guide to Foreman community.

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.12

Copy link

github-actions bot commented Aug 12, 2024

The PR preview for 1cf4cf8 is available at theforeman-foreman-documentation-preview-pr-3212.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@Lennonka Lennonka force-pushed the upstreamize-api-guide branch from 70d7ab1 to 849002e Compare August 12, 2024 23:38
@Lennonka

This comment was marked as outdated.

@Lennonka Lennonka mentioned this pull request Aug 13, 2024
3 tasks
@Lennonka Lennonka marked this pull request as ready for review August 19, 2024 23:58
@Lennonka
Copy link
Contributor Author

Now is the good time to address any changes in the content. Afterwards, I will move the assemblies and modules to common/*.

@Lennonka Lennonka force-pushed the upstreamize-api-guide branch from a916347 to f8b2dfa Compare August 27, 2024 12:18
@Lennonka
Copy link
Contributor Author

Rebased.

@Lennonka Lennonka force-pushed the upstreamize-api-guide branch from f8b2dfa to 6cda27b Compare August 27, 2024 12:20
@Lennonka
Copy link
Contributor Author

Dropped fake Hammer URL attribute.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

26/52. Looks good so far; only minor suggestions.

guides/common/attributes-base.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/master.adoc Show resolved Hide resolved
guides/doc-Project_API/master.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/topics/con_api-cheat-sheet.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/topics/con_api-cheat-sheet.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

some more suggestions. I did not yet test any of the scripts, but I wonder in general: Can we move certain Python/Ruby/Bash scripts from foreman-documentation to foreman/foreman_maintain? I am unsure if it's a good idea to keep that much code in the docs git repo. (not blocking your PR; could be a future RFC/RFE)

51/52 done. API permissions matrix left.

[id="applying-errata-to-a-host-or-host-collection"]
= Applying errata to a host or host collection

You can use the API to apply errata to a host, host group, or host collection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Intro does not match anchor/title/file name.

Maybe a generic title such as "Applying errata to hosts by using {Project} API"?

I also wonder: how should be name these files in general? I assume there's already a file "proc_applying-errata-to-hosts.doc" which houses the WebUI/Hammer CLI procedures.

  • What's our (future!) convention with titles/file names/anchors?
  • Should we add the API procedure to the "normal"/existing procedure? Aka. is it a good idea to split off procedure by means to interacting with Foreman?

cc @asteflova + @apinnick

FYI: not blocking your PR!

Copy link
Member

Choose a reason for hiding this comment

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

* Should we add the API procedure to the "normal"/existing procedure? Aka. is it a good idea to split off procedure by means to interacting with Foreman?

That's a good point. I can imagine that a more thorough review of how to actually merge the new API modules with the existing guides will be required (what I can't imagine is who would have the capacity for it in the foreseeable future 🙃). For example, some time ago, I used the API guide's overview module and added it (with a few edits) to the Overview guide: https://docs.theforeman.org/nightly/Planning_for_Project/index-katello.html#API-Overview_planning This module will now effectively be duplicated but it works well within the Overview guide. There may be more modules like that that would fit well in the existing guides.

Adding the API procedures from the API guide to appear alongside the CLI and web UI procedures (and also Ansible procedures) seems like a good idea. It would take a lot of effort, though.

guides/doc-Project_API/topics/proc_working-with-hosts.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/topics/proc_working-with-hosts.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/topics/proc_working-with-pulp.adoc Outdated Show resolved Hide resolved
guides/doc-Project_API/topics/proc_working-with-pulp.adoc Outdated Show resolved Hide resolved
@Lennonka
Copy link
Contributor Author

Lennonka commented Aug 28, 2024

Re. API permission matrix

We can use a script similar to this to generate the table: https://github.com/theforeman/foreman-documentation/blob/master/guides/common/modules/proc_creating-a-complete-permission-table.adoc

Or we can just link to it or include it instead 👀

cc @maximiliankolb @asteflova

@Lennonka
Copy link
Contributor Author

Lennonka commented Sep 2, 2024

I fixed some Vale errors. The remaining ones look like false positives to me. Feel free to check.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

31/49

almost all comments are mere suggestions and could also get applied in a follow-up PR

guides/common/modules/proc_applying-errata-to-hosts.adoc Outdated Show resolved Hide resolved
guides/common/modules/proc_calling-the-api-in-curl.adoc Outdated Show resolved Hide resolved
guides/common/modules/proc_calling-the-api-in-curl.adoc Outdated Show resolved Hide resolved
These API calls require that you pass data in `json` format with the API call.
For more information, see xref:sect-API_Guide-Passing_JSON_Data_with_the_API_Request[].

[id="ex-API_Guide-Creating_a_New_User"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[id="ex-API_Guide-Creating_a_New_User"]
[id="creating-a-user"]

looking at this anchor, I wonder if it makes sense to prefix all those anchors with either "api-" or "api-example-"; or add a "-through-api" or "-by-using-{project-context}-api" suffix?

I lean towards a short and sweet "api-" prefix similar to CLI procedures. Then, when we eventually move (or not!) those examples to the "creating users" procedure, the anchors will align/not break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of api- prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these anchors aren't that important. Please, change them in a follow-up PR.

== Using apipie bindings with Ruby

Apipie bindings are the Ruby bindings for apipie documented API calls.
They fetch and cache the API definition from {Project} and then generate API calls on demand.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
They fetch and cache the API definition from {Project} and then generate API calls on demand.
They fetch and cache the API definition from {Project} and then generate API calls on-demand.

@apinnick I'm not sure what is correct in this case. Can you please weigh in?

Co-authored-by: Maximilian Kolb <[email protected]>
@Lennonka
Copy link
Contributor Author

Lennonka commented Sep 3, 2024

@maximiliankolb I hesitate to modify the IDs without checking xrefs. I'd prefer to change them in another PR.

Comment on lines +14 to +23
By default, {Project} authorizes all OAuth API requests as the built-in anonymous API administrator account.
Therefore, API responses include all {Project} data.
However, you can also specify the {Project} user that makes the request and restrict access to data to that user.

To enable OAuth user mapping, enter the following command:

[options="nowrap", subs="+quotes,attributes"]
----
# {foreman-installer} --foreman-oauth-map-users true
----
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC deploying a new content proxy relies on oauth and I'm not sure what enabling user mapping does to it, if anything. @ekohl could you please chime in?

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

39/49. Will do the rest tomorrow. Let's do headings and minor formatting/emphasis in a follow-up PR.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

41/49 done; will finish this tomorrow.


[id="proc-API_Guide-Uploading_Content_to_the_{Project}_Server"]
.Procedure
. Assign the package name to the variable `name`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the trailing colon on 17; and similar on 25 and others.

Alternative suggestion but much more work: get rid of "Example request" everywhere. It's an API documentation; we're not wizards: of course it's an example.

Not blocking.

@Lennonka
Copy link
Contributor Author

Lennonka commented Sep 5, 2024

I would prefer further changes in subsequent PRs.

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

49/49 🎉

I found many minor cosmetic suggestions from anchors, cli argument orders, example values, emphasis, etc. None of this blocks, in my perspective, this PR. As discussed on Monday, it's been part of Satellite downstream docs as-is for a long time. Therefore, I propose to merge as is and then iterate.

@Lennonka I am OK if you decide on a per-thread basis to apply more changes or to keep it as is. My only concern is that we need to track all SME feedback from this PR.

I would be happy to start working on all those things next week.

@Lennonka
Copy link
Contributor Author

Lennonka commented Sep 6, 2024

I am OK if you decide on a per-thread basis to apply more changes or to keep it as is. My only concern is that we need to track all SME feedback from this PR.

Let's create some Issues :)

@Lennonka Lennonka merged commit d7fbafe into theforeman:master Sep 6, 2024
8 of 9 checks passed
@Lennonka Lennonka deleted the upstreamize-api-guide branch September 6, 2024 09:06
Lennonka added a commit that referenced this pull request Sep 6, 2024
Co-authored-by: Maximilian Kolb <[email protected]>
@Lennonka
Copy link
Contributor Author

Lennonka commented Sep 6, 2024

Cherry-picked:

@maximiliankolb
Copy link
Contributor

Thanks Lena!

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.

5 participants