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

Add CLI procedure for syncing content view #3235

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bangelic
Copy link
Contributor

A CLI procedure for synchronizing a content view has been added to the
Content Management Guide related to
https://issues.redhat.com/browse/SAT-13027.

  • 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.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

Copy link

github-actions bot commented Aug 22, 2024

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.

hammer proxy content synchronize is present on Foreman 3.11 ✔️

However, I think we should drop the output from "--help" and instead add a short description of what it does and which options are required.

I also have concerns about the title: It indicates that you can synchronize remote content to Foreman based on content views. However, the Hammer CLI command looks like it's about synchronizing CVs from Foreman Server to Smart Proxy Servers based on CVs. Please clarify or correct me.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Aug 23, 2024
Copy link
Contributor

@asteflova asteflova left a comment

Choose a reason for hiding this comment

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

@bangelic Can you provide more details about why you are adding this? For example, if this is a new command, I would argue that you don't need a full section (just a release note could be enough). If there is a customer case that requests documenting the command, you could just mention it in another section (preferably in a procedure that customer was following when they hit the issue).

Without further clarification as to where the request to add the command comes from, I would not support adding a section like this. I find it hard to imagine a scenario where adding this section would do more than simply duplicate a --help output.

@ianballou
Copy link
Contributor

ianballou commented Aug 29, 2024

@asteflova I think some documentation mentioning that this functionality is available past a release note would be helpful, it's a unique feature in that you can only do it via hammer. Users might not think to look in the hammer help text to discover that the feature is possible.
If an entire new section is too much, how about mentioning it (without too many details?) wherever we document syncing smart proxies in general?

@asteflova
Copy link
Contributor

@asteflova I think some documentation mentioning that this functionality is available past a release note would be helpful, it's a unique feature in that you can only do it via hammer. Users might not think to look in the hammer help text to discover that the feature is possible.

Fair enough, thanks for the details.

If an entire new section is too much, how about mentioning it (without too many details?) wherever we document syncing smart proxies in general?

That could definitely work. If we have a procedure during which users might need that new command, it would be good to mention it there.

If we don't have an existing procedure like that, I think the new command should also appear in the Hammer CLI guide, right? So a release note could link to the appropriate section in the Hammer CLI guide.

@bangelic bangelic force-pushed the bangelic-SAT-13027-How-to-sync-a-content-view-CLI-procedure branch from 856b383 to defd3c5 Compare September 6, 2024 17:28
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Sep 6, 2024
@bangelic
Copy link
Contributor Author

bangelic commented Sep 6, 2024

hammer proxy content synchronize is present on Foreman 3.11 ✔️

However, I think we should drop the output from "--help" and instead add a short description of what it does and which options are required.

I also have concerns about the title: It indicates that you can synchronize remote content to Foreman based on content views. However, the Hammer CLI command looks like it's about synchronizing CVs from Foreman Server to Smart Proxy Servers based on CVs. Please clarify or correct me.

@maximiliankolb I think that @ianballou will be able to clarify but he will be on PTO until 9/13. Will check with him when he returns.

@ianballou
Copy link
Contributor

ianballou commented Sep 17, 2024

However, I think we should drop the output from "--help" and instead add a short description of what it does and which options are required.

I think this would be covered by the short discussion I had with @asteflova above. No need for the help text, but an addition to the smart proxy syncing guide that mentions hammer can be used to sync content views (and even repositories!) from Foreman to smart proxies via hammer {hammer-smart-proxy} content synchronize. I don't think more details are needed since the user can use the hammer help text.

I also have concerns about the title: It indicates that you can synchronize remote content to Foreman based on content views. However, the Hammer CLI command looks like it's about synchronizing CVs from Foreman Server to Smart Proxy Servers based on CVs. Please clarify or correct me.

I agree. This comment might have different context now based on what I mentioned above, but just make sure it's clear the syncing is from Foreman to smart proxies.

@bangelic bangelic force-pushed the bangelic-SAT-13027-How-to-sync-a-content-view-CLI-procedure branch from defd3c5 to 37ed367 Compare September 27, 2024 19:21
@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label Oct 16, 2024
@bangelic bangelic force-pushed the bangelic-SAT-13027-How-to-sync-a-content-view-CLI-procedure branch from 37ed367 to 4b4b514 Compare October 25, 2024 20:32
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants