sat-3016 - Added/Edited new chapter/assembly about foreman webhooks #636
Conversation
|
Hello and welcome. I am... confused... - we have also PR #612 What is occurring :o) |
Hey @neal-timpe thank you for clarifying. If you haven't set yourself up for local tests, please follow: https://github.com/theforeman/foreman-documentation/blob/master/guides/README.md#building I appreciate you helping out! |
|
@melcorr Yes, I set up the tools and ran a build. It looks okay to me and the links work. I also went back through to make sure there was one sentence per line. What else do you see? Which items did I miss? |
There was a problem hiding this comment.
Hey @neal-timpe - thanks so much.
I pointed to the attribute that I immediately saw and had pointed out in the other PR. There is no way this can build correctly.
Now you have confirmed you have built locally, I will review this in the morning.
I appreciate you getting back to me and your contribution.
Thanks,
Melanie
There was a problem hiding this comment.
Hey Neal,
Firstly, this is a great first contribution and thank you for reacting to my earlier comments, for confirming that you have built locally, and for implementing the initial feedback. It's not easy to come into a project half way through a work in progress. I appreciate your efforts and responsiveness.
Overall, we are very much on track with this.
My major concern is that the information about webhooks and shellhooks should be treated separately and have separate procedures.
The engineer's notes probably had them jumbled together, but this isn't really standard throughout our docset. You could either clone the files (easiest) and create shellhooks procedures, or conditionalise the files and have them print twice depending on the context set. As it is now, in the organization of the chapter. This really doesn't seem consistent with the rest of our docs. However, it's an easy enough thing to fix!
If you're unsure about anything, let me know.
@ofedoren - in the absence of lzap, would you mind having a quick look through these procedures? If you can, could you confirm how foreman_hooks is referred to in the downstream? Can you give a general + or - to these docs?
guides/common/modules/proc_installing-webhooks-shellhooks-plugin.adoc
Outdated
Show resolved
Hide resolved
| # {foreman-installer} --enable-foreman-plugin-webhooks | ||
| ---- | ||
|
|
||
| * Optionally, install the shellhooks plugin on each {SmartProxy} used for shellhooks, using the following command: |
There was a problem hiding this comment.
I find it so strange that in the introduction that we do not provide any info about shellhooks, yet we are telling people they can install it straight away. Seems a bit inconsistent for users?
There was a problem hiding this comment.
I agree. What if we added a sentence that said, for more information, see the shellhooks and have an xref to the shellhooks procedure.
|
@melcorr I've addressed your comments. I'm not sure if I've chosen the correct title for the Shellhooks reference topic. I chose Shellhooks usage details. I acknowledge that may not be it, but I wanted to move forward. Anyway. I think I'm ready for another look. I also responded to one of your comments that is unresolved. I wanted to ask if that's an acceptable solution before I did it. Thanks! |
|
@neal-timpe I will review this in the morning and get back to you! |
|
Just for the record, I cloned your fork to a fresh folder, downloaded, checked your logs, and still this does not look right. Edit: as in I tried it again, at the end of my day |
|
@melcorr I'm not sure what to check here. What doesn't look right about it? I have built locally and it looks fine to me. What should I be looking for? |
There was a problem hiding this comment.
Hey @neal-timpe,
Apologies.
In the end, this was my bad.
To explain: I tried a few times yesterday and was convinced something was wrong: I believed that items in an earlier commit from you were overwritten.
This morning, when I looked again, I saw that I was just thrown by a few items that I was convinced you had changed in your commits that were in fact not updated. I've flagged them.
Elise wrote to me yesterday that you have limited availability to continue working on this.
Not merging things that are incomplete is a standard practice in both docs and code, so I'm quite bewildered as to how to proceed.
Traditionally, when we have worked on feature docs, we have often gone through a few reviews because as the information is clarified, some other holes are revealed.
For you, I've made a number of comments for additional small changes, but as you are aware the shellhooks issue still exists.
I think that one thing that would help would be to move all the conceptual topics above the procedure for installing webhooks in the chapter.
I'm hoping that in Lukas's absence that @ares or @ofedoren might add some insight here that might then clear up the reasons for having webhooks and shellhooks referenced throughout so that we can better explain it to users. I think we are missing vital context here.
We have a section talking about migration from shellhooks to webhooks.
We have nothing about the advantages of working with combined webhooks and shellhooks yet the installation procedures are combined.
Do we want users to blend shellhooks and webhooks?
Should they be treated separately ?
Do we think users should install both simultaneously?
Should we be encouraging users to move entirely to webhooks and therefore talk about migration but move away from shellhooks (aka delete references to installing them)?
|
@melcorr, I'll try to answer those questions:
That's quite wrong. We don't migrate from
It's worth to mention that
Well, depends on users' use-cases.
I guess yes, only if they want similar functionality as with
We should encourage users to move to |
|
@melcorr I've done my best to incorporate the feedback that you gave. Thanks for taking the time to look through this again. I'm just filling in and I'm not on the Satellite team, so I don't know the answers to your questions. I'm not sure if I need to make more changes based on what @ofedoren said. I agree and I don't want you to merge things you feel aren't ready. However, my role here is to make changes to someone else's content. I'm happy to make more changes to this if you feel they are necessary. I also don't want to say I can't, because finding the answers to these questions is possible, but it requires me joining the team and doing a bunch more research. |
|
Hey @neal-timpe |
| + | ||
| A webhook HTTP payload must be created using {Project} template syntax. | ||
| + | ||
| For more information, see {ManagingHostsDocURL}appe-red_hat_satellite-managing_hosts-template_writing_reference[Template Writing Reference] in _Managing Hosts_ and for available template macros and methods, visit `/templates_doc` on {ProjectServer}. |
There was a problem hiding this comment.
The anchor has been changed, see guides/doc-Managing_Hosts/topics/Template_Writing.adoc:4:[[appe-Managing_Hosts-Template_Writing_Reference]]
| For more information, see {ManagingHostsDocURL}appe-red_hat_satellite-managing_hosts-template_writing_reference[Template Writing Reference] in _Managing Hosts_ and for available template macros and methods, visit `/templates_doc` on {ProjectServer}. | |
| For more information, see {ManagingHostsDocURL}appe-Managing_Hosts-Template_Writing_Reference[Template Writing Reference] in _Managing Hosts_ and for available template macros and methods, visit `/templates_doc` on {ProjectServer}. |
| [id="creating-a-webhook_{context}"] | ||
| = Creating a Webhook | ||
|
|
||
| You can customize events, payloads, HTTP authentication, content type and headers through {ProjectWebUI}. |
There was a problem hiding this comment.
| You can customize events, payloads, HTTP authentication, content type and headers through {ProjectWebUI}. | |
| You can customize events, payloads, HTTP authentication, content type, and headers through {ProjectWebUI}. |
See IBM Style Guide: 'Commas between items in a series'. Can you confirm this @melcorr ?
| . Click *Create Webhook*. | ||
| . Click *Subscribe to* to select an event. | ||
| . Enter a name. | ||
| . Enter a target URL. The target URL can be a dynamic URL. |
There was a problem hiding this comment.
| . Enter a target URL. The target URL can be a dynamic URL. | |
| . Enter a target URL. | |
| The target URL can be a dynamic URL. |
| . Click *Subscribe to* to select an event. | ||
| . Enter a name. | ||
| . Enter a target URL. The target URL can be a dynamic URL. | ||
| When using the shellhooks plugin, the URL should be in the form of `https://{smartproxy-example-com}:{smartproxy_port}/shellhooks/my_script`. |
There was a problem hiding this comment.
| When using the shellhooks plugin, the URL should be in the form of `https://{smartproxy-example-com}:{smartproxy_port}/shellhooks/my_script`. | |
| When using the shellhooks plugin, the URL should be in the form of `\https://{smartproxy-example-com}:{smartproxy_port}/shellhooks/my_script`. |
Adding the backslash make the link "unclickable" in asciidoc.
| . Enter an HTTP method. | ||
| . Check the *Enabled* flag if you want to create an active webhook. | ||
| . Click the *Credentials* tab. | ||
| . Optional: Enter the username and password when HTTP authentication is required. |
There was a problem hiding this comment.
| . Optional: Enter the username and password when HTTP authentication is required. | |
| . Optional: If HTTP authentication is required, enter the username and password. |
IBM Style Guide: purpose before action.
|
|
||
| The scope of what is available is limited by the safemode and all objects and macros are both subject to API stability promise and are fully documented. | ||
|
|
||
| The number of events triggered by webhooks is substantially less than with `foreman_hooks`. |
There was a problem hiding this comment.
| The number of events triggered by webhooks is substantially less than with `foreman_hooks`. | |
| The number of events triggered by webhooks is substantially fewer than with `foreman_hooks`. |
50% sure, but not a native speaker. @melcorr
|
|
||
| The number of events triggered by webhooks is substantially less than with `foreman_hooks`. | ||
|
|
||
| {Project} are designed to be safe. |
There was a problem hiding this comment.
Could you please rephrase this? I am unsure what's the purpose of this statement.
| {Project} are designed to be safe. | ||
| Webhooks are processed asynchronously so there is minimal risk of tampering with internals of the system. | ||
| It is not possible to migrate from `foreman_hooks` without creating payloads for each individual webhook script. | ||
| However, the webhooks plugin comes with several example payload templates. You can use the example payloads with shellhooks to simplify migration. |
There was a problem hiding this comment.
| However, the webhooks plugin comes with several example payload templates. You can use the example payloads with shellhooks to simplify migration. | |
| However, the webhooks plugin comes with several example payload templates. | |
| You can use the example payloads with shellhooks to simplify migration. |
| @@ -0,0 +1,17 @@ | |||
| [id="shellhooks_{context}"] | |||
| = Shellhooks usage details | |||
There was a problem hiding this comment.
| = Shellhooks usage details | |
| = Shellhooks Usage Details |
| [id="shellhooks_{context}"] | ||
| = Shellhooks usage details | ||
|
|
||
| You can install a {SmartProxy} Shellhooks that expose executables using a REST HTTPS API. |
There was a problem hiding this comment.
Do you mean "shellhooks plugin"?
|
Feel free to reopen this at any time! |
Edited previous work on foreman webhooks.
[doc] Foreman Webhooks
https://issues.redhat.com/browse/SAT-3106
Cherry-pick into: