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

Added create credentials/inventories, github app token #92

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from

Conversation

tvo318
Copy link
Member

@tvo318 tvo318 commented Feb 26, 2025

This PR achieves the following:

  1. Adding an inventory, adding an inventory source (AAP-39252)
  2. Removing all inventories (and inventory sources) except for those in AWX (based on ANSTRAT-836)
  3. Adding a credential (AAP-39252)
  4. Removing all credentials (and credential plugins) except for those in AWX (based on ANSTRAT-836)
  5. Adding the GitHub App Install token procedure (AAP-39191)

See rendered (staged) content here: https://awx-plugins-core--92.org.readthedocs.build/en/92/

@tvo318 tvo318 requested review from arrestle and webknjaz February 26, 2025 15:37
@tvo318 tvo318 requested a review from arrestle February 26, 2025 23:36
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like an image to me...

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, where should I put it?

Copy link
Member

Choose a reason for hiding this comment

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

Dunno, depending on how it's used.

@webknjaz
Copy link
Member

That's a huge chunk of changes. Perhaps, split this into smaller PRs doing one thing at a time?

@tvo318
Copy link
Member Author

tvo318 commented Feb 28, 2025

@webknjaz I agree it is big, but it was existing content that had been reviewed and approved before. And much of it is removing content. Maybe we can just focus on the new credential plugin (GitHub app) and get this PR merged and if we need to make additional changes, we can open follow-on PRs?

@webknjaz
Copy link
Member

@tvo318 we can't merge this because it breaks the CI and mustn't introduce linting violations. It'd be easier to review parts of it like removal. Otherwise, don't expect a quick review timeline, since this now requires substantial time investment to review.

.. _ug_inventories_add_host:

Add hosts
------------
Copy link
Member

Choose a reason for hiding this comment

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

the underline must match the text of the title

Inventory Sources
^^^^^^^^^^^^^^^^^^^^^^

To configure inventory sources, refer to `ug_inventories_plugins`.
Copy link
Member

Choose a reason for hiding this comment

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

backticks

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Looks like there's a lot of title mismatches, which is something you have to fix for RST to be happy. I've also seen a number of places with unspecified highlighting. Some would benefit from yaml+jinja. Others need console (for shell session snippets).
There's one place with single backticks that the linters caught, a yamllint issue (the file must be edited to match the linter config). Also, a misplaced YAML file in the directory layout — it shouldn't be in the images directory because it's a text file and not an image. Where is it even used?

This is just off the top of my head. There's probably some other blockers.

.. _ug_inventory_sources:

Inventory Sources
^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

underline mismatch

.. _ug_customscripts:

Export old inventory scripts
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

another mismatch

@webknjaz
Copy link
Member

@tvo318 could you fix the PR title to be in imperative mood and reference to one single topic per typical Git commit best practices?

@webknjaz
Copy link
Member

It should probably clarify that you're adding the docs and aren't deleting actual code…

Comment on lines +6 to +9
.. index::
single: credentials
pair: credentials; multi
pair: credentials; assignment
Copy link
Member

Choose a reason for hiding this comment

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

What does this do?

Copy link
Member

Choose a reason for hiding this comment

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

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#index-generating-markup implies this is for some references but I don't see where it's used...


You can verify functionality with ``ansible-inventory``. This should give the same data, but reformatted.

.. code-block:: bash
Copy link
Member

Choose a reason for hiding this comment

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

This isn't bash. It's actually console.


Each file contains a script. Scripts can be ``bash/python/ruby/more``, so the extension is not included. They are all directly executable (assuming the scripts worked). If you execute the script, it dumps the inventory data.

.. code-block:: bash
Copy link
Member

Choose a reason for hiding this comment

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

The console lexer is supposed to be used for terminal sessions with prompts and outputs.

Despite the removal of the custom inventory scripts API, the scripts are still saved in the database. The commands described in this section allows you to recover the scripts in a format that is suitable for you to subsequently check into source control. Usage looks like this:


::
Copy link
Member

Choose a reason for hiding this comment

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

console?


To configure AWX:

1. To create a service account, you may download and use this sample service account, :download:`containergroup sa <_static/images/containergroup-sa.yml>` and modify it as needed to obtain the above credentials.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this where it's used? Create some other folder in the static dir, then. Maybe name it “samples” or something.


1. To create a service account, you may download and use this sample service account, :download:`containergroup sa <_static/images/containergroup-sa.yml>` and modify it as needed to obtain the above credentials.

2. Apply the configuration from ``containergroup-sa.yml``::
Copy link
Member

Choose a reason for hiding this comment

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

Use the :file: role for things that look like file/dir paths.


5. Get the CA cert::

oc get secret $SA_SECRET -o json | jq '.data["ca.crt"]' | xargs | base64 --decode > containergroup-ca.crt
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be highlighted as bash. Or console if you add a $-prompt prefix.

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.

3 participants