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

fix(docs): add RBAC and user to executor plugins example #13019

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dgolja
Copy link

@dgolja dgolja commented May 8, 2024

Fixes #13015

Motivation

Having a working example from the documentation.

Modifications

The main practical modification involves updating the user ID. Other changes, especially the RBAC role is to provide some guidance what permissions you need for the example to work (see).

Verification

Example provided in the documentation now works as expected.

…n, patch example and correct user ID.

Signed-off-by: Dejan Golja <dejan@golja.org>
@agilgur5 agilgur5 changed the title fix(doc): Update executor plugins example fix(docs): add RBAC and user to executor plugins example May 8, 2024
@agilgur5 agilgur5 added area/docs Incorrect, missing, or mistakes in docs area/plugins area/agent Argo Agent that runs for HTTP and Plugin templates area/executor labels May 8, 2024
@agilgur5 agilgur5 self-assigned this May 8, 2024
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

A small copy-edit below and also a suggestion to unify Agent permissions docs

docs/executor_plugins.md Show resolved Hide resolved
@@ -202,6 +211,30 @@ spec:

You'll see the workflow complete successfully.

**Note**: The service account running the workflow needs at least the following permissions. If <= v3.2 you must replace `workflowtasksets/status` with `workflowtasksets`.
Copy link
Member

Choose a reason for hiding this comment

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

I think this block should go in the ## Configuration section above a subsection ### Permissions. It's more important than a "Note" -- this is required

Copy link
Member

Choose a reason for hiding this comment

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

The Argo "agent" is mentioned in the HTTP template docs as well. Ideally we'd unify them and have a page on the Agent that both link to with regard to permissions / more information. The Agent is poorly documented; I didn't even know much about it until a few months ago

Potentially a section in the existing Workflow RBAC page might make sense, which is what the quick-start manifest already links to

Copy link
Member

@agilgur5 agilgur5 May 8, 2024

Choose a reason for hiding this comment

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

I would also leave the "If <= v3.2" out of this and leave it in the Workflow description annotation like the quick start manifest. Let's keep the two identical.

Even better would be to make that an in-line comment on the workflowtasksets/status line in both

Copy link
Author

Choose a reason for hiding this comment

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

Let me know what you think about the latest change. I added the ### Permissions section with some relevant links and removed the Role definition. Not every user may experience permission issues, because it depends on how they installed WF. I think this new section letting them know they may need to adjust the permissions are adequate for now.

I agree that the documentation for the agent and permissions, in general, could be better. Let me think about it a bit more, and for now, I'll provide this as a stopgap. I may create a follow-up PR to improve the permissions documentation, but first, I need to dig more into the code to fully understand it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this new section letting them know they may need to adjust the permissions are adequate for now.

Yea we'd still leave this section or something similar if we move the actual Agent docs to its own page; this section would just link to that, something like "Plugins use the Argo Agent, ensure you have appropriate RBAC for your Workflow".

Let me know what you think about the latest change.

It looks like the current text is mostly just a copy of the HTTP Template doc? That section causes confusion as-is the way it is written so if we're going to add it, I'd prefer to unify in the Workflow RBAC page. We can just have a short description there and show the RBAC. Can improve that in later PRs

I can help guide that if you're ok relying on my knowledge.

docs/executor_plugins.md Outdated Show resolved Hide resolved
Signed-off-by: Dejan Golja <dejan@golja.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agent Argo Agent that runs for HTTP and Plugin templates area/docs Incorrect, missing, or mistakes in docs area/executor area/plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

executor plugins example from documentation does not work
2 participants