-
Notifications
You must be signed in to change notification settings - Fork 85
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
Use commands for container push #3023
base: master
Are you sure you want to change the base?
Use commands for container push #3023
Conversation
The PR preview for b9dc10d is available at theforeman-foreman-documentation-preview-pr-3023.surge.sh The following output files are affected by this PR: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass at a review :) I'm making a few suggestions that would help integrate the new additions more organically with the existing content of the module you're updating. Let me know if I'm misinterpreting something.
a397e98
to
790cdb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @bangelic, the changes you've made so far made it a lot easier for me to understand the context of your addition more clearly.
In this round of feedback, I'm offering mostly specific suggestions, but I also have a couple of open questions in places where I had trouble understanding what was being described.
* Content can only be pushed to the {Project} itself. | ||
* If pushed content needs to be on {SmartProxies} as well, use {SmartProxy} syncing. | ||
* Podman requires that all characters in the pushed container name contain only lowercase characters. | ||
* To push content to an organization or product, a user must be able to create repositories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen information about user permissions included under a .Prerequisite
heading in other modules. Can you create a heading like that and move this line there?
Also, how do I know that my user is able to create repositories? Are we talking about Foreman roles/permissions? Which ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianballou Does a user know if they have permissions to create repositories? Specific role or permissions needed? I am pushing a commit to say that a user must have permissions to create repositories in order to push content to an organization or product.
* A repository in Katello is created once the content is pushed. | ||
* If the organization or product does not exist, Katello sends an error telling the user to ensure the proper entities are created first. | ||
* Subsequent pushes to the same repository updates the contents of that repository. | ||
* Content is pulled as soon as it is pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is curious. The procedure mentions pulling container images manually. Does this mean that when you push a container image, that image gets automatically pulled afterwards?
I don't know what to do about this sentence, but I felt the need to mention that it confused me in the context of the whole module 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianballou Any chance you can provide some clarity here?
Writing manifest to image destination | ||
---- | ||
|
||
.Limitations on container registries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this subheading for it to be placed somewhere closer to the beginning of the module? I don't think the information here will be relevant after .Procedure
, after users have run the command(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, feel free to reevaluate whether including the limitations as bullet points still works if/after you implement my suggestions. It's quite possible that after the edits, regular paragraphs will make more sense.
Containers are able to be pushed to Satellite's container registry. This command for pushing containers was needed to show the user how to do so.
Moved Limitation section. Added attribute for foreman example. Removed bullet points for continuing sentences. Addressed some style issues.
790cdb5
to
b9dc10d
Compare
Containers are able to be pushed to Satellite's
container registry.
This command for pushing containers was needed to show the user how to
do so.
Please cherry-pick my commits into: