Use upstream terminology #3351
Use upstream terminology #3351
Conversation
| # URL for the API to your deployed {ProjectX} server | ||
| {project-allcaps}_API = f"\{URL}/katello/api/v2/" | ||
| # Katello-specific API | ||
| FOREMAN_API = f"\{URL}/katello/api/v2/" |
There was a problem hiding this comment.
| FOREMAN_API = f"\{URL}/katello/api/v2/" | |
| FOREMAN_API = f"\{URL}/api/" |
There was a problem hiding this comment.
Good catch. The URL was borked :D
There was a problem hiding this comment.
Bo-what? :D However, I'm not sure how this change will impact the rest of the script.
There was a problem hiding this comment.
The thing is that I wonder about the following:
{FOREMAN_API}/organizationsshould be okay- but I guess that you need Katello to work with environments
{KATELLO_API}/organizations/{org_id}/environments/
Let me verify this in the API doc.
maximiliankolb
force-pushed
the
use_upstream_terminology
branch
from
5940328 to
1b834d0
Compare
pr-processor
bot
added
the
Waiting on contributor
pr-processor
bot
added
Needs re-review
and removed
Waiting on contributor
There was a problem hiding this comment.
Thanks for checking; I've applied both your suggestions. @Lennonka
| SATID=$(hammer --csv template list --per-page=1000 | grep "provision" | grep ",Kickstart default" | cut -d, -f1) | ||
| PARTID=$(hammer --csv partition-table list | grep "{client-provisioning-template-type} default," | cut -d, -f1) | ||
| PXEID=$(hammer --csv template list --per-page=1000 | grep "{client-provisioning-template-type} default PXELinux" | cut -d, -f1) | ||
| {project-allcaps}_ID=$(hammer --csv template list --per-page=1000 | grep "provision" | grep ",{client-provisioning-template-type} default" | cut -d, -f1) |
There was a problem hiding this comment.
Previously it was named SAT_ID but, isn't this really the TEMPLATE_ID or even PROVISION_TEMPLATE_ID?
Also, this really goes beyond the scope of this PR but I really dislike that it's using grep to "search" in combination with a very large per page number.
I don't have a setup at hand to test it and there there may be a better way, but I'd suggest to try
| {project-allcaps}_ID=$(hammer --csv template list --per-page=1000 | grep "provision" | grep ",{client-provisioning-template-type} default" | cut -d, -f1) | |
| PROVISION_TEMPLATE_ID=$(hammer --csv --no-headers template list --search 'name = "{client-provisioning-template-type} default"' --fields=Id --per-page 1) |
Perhaps @ofedoren has some better recommendation.
There was a problem hiding this comment.
Input on improving the templates is very much appreciated. For this PR, it is out of scope but I will keep this thread open and apply feedback later if necessary.
I have two possible follow-up changes in mind:
- Extract the actual code into snippets with a specific naming scheme, for example,
snip_code_x-y.adocand write some minor automation to at least lint the code using flake8/black/rubocop. Next step would be to automatically run this code on a Foreman/Katello instance. - Deliver this code, similar to
bin/katello-certs-checkin foreman-installer, via RPM. We actually spoke about automating a procedure, I believe it was Salt or Ubuntu-related, in the past @ekohl.
There was a problem hiding this comment.
I'd prefer to recommend foreman-ansible-modules (or Red Hat's commercially supported branded equivalent). If you look at theforeman/forklift#1856 you can see it's way easier to maintain.
There was a problem hiding this comment.
Yes, FAM works great. We use that downstream/internally quite a bit. However, as part of the Foreman API guide, I don't think we want to show FAM examples (even though FAM also uses the Foreman/Katello API internally). I think a FAM example would be nice in https://docs.theforeman.org/nightly/Administering_Project/index-katello.html#Managing_Project_with_Ansible_Collections_admin
Is it OK to open an issue and move this discussion/implementation to another issue/PR?
There was a problem hiding this comment.
Hammer and FAM both use the API. If you're take that position, I think Hammer shouldn't be in the API guide either. But it's not: https://docs.theforeman.org/nightly/Provisioning_Hosts/index-katello.html#updating-the-details-of-multiple-operating-systems_provisioning
I think guiding users to FAM instead of writing shell scripts is a better solution, but agree it's out of scope for this PR.
maximiliankolb
added
the
style review done
* Use attributes in favor of branded variable names * Use attribute in favor of Kickstart * Rename variable in Python script * Fix Foreman API URL for Python example script * Fix attribute for Katello API URL Possible follow-up tasks: * Rename more variables in Python scripts * Use buillt-in search in Hammer in favor of grep in the result (cherry picked from commit e063255)
What changes are you introducing?
Use attributes in favor of hardcoded branded variable names.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
Fixes branding issues upstream+downstream.
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Checklists
Please cherry-pick my commits into: