-
Notifications
You must be signed in to change notification settings - Fork 124
initial pulp doc #523
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
base: main
Are you sure you want to change the base?
initial pulp doc #523
Conversation
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.
Code Review
This pull request introduces new documentation for getting access to Pulp storage. The document is a great starting point, providing a clear overview and helpful examples. My review focuses on aligning the document with the Konflux Docs Style Guide. Specifically, I've made suggestions to adopt a more professional and formal tone, and to consistently use the imperative mood for instructions, as required by the style guide. I also identified a YAML code snippet that could be confusing for users and recommended clarification. Addressing these points will improve the document's quality and adherence to project standards.
| [source,yaml] | ||
| ---- | ||
| - name: rpmbuild-and-push | ||
| image: quay.io/fedora/fedora:latest | ||
| script: | | ||
| #!/bin/bash | ||
| set -e | ||
|
|
||
| # Build your RPM | ||
| rpmbuild -ba mypackage.spec | ||
|
|
||
| # Configure pulp-cli from the mounted secret | ||
| export PULP_CLI_CONFIG=/workspace/pulp-config/cli.toml | ||
| export PULP_CERT=/workspace/pulp-config/tls.crt | ||
| export PULP_KEY=/workspace/pulp-config/tls.key | ||
|
|
||
| # Upload to Pulp | ||
| pulp rpm upload ~/rpmbuild/RPMS/x86_64/*.rpm | ||
| volumeMounts: | ||
| - name: pulp-config | ||
| mountPath: /workspace/pulp-config | ||
| readOnly: true | ||
| volumes: | ||
| - name: pulp-config | ||
| secret: | ||
| secretName: pulp-access | ||
| ---- |
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 YAML snippet is structurally confusing. It appears to be a mix of a step definition and a volumes definition from a pod template, but they are not correctly nested. This can be misleading for users. For clarity, please either provide a more complete and valid example (e.g., a minimal Tekton Task) or add a note explaining that these are fragments and show how they should be structured within a valid resource.
| Pulp is our artifact storage system for everything that's not a container image. Think RPMs, Python wheels, Go modules, generic files - basically any build artifact that doesn't belong in an OCI registry. While container images go to Quay, everything else should land in Pulp. | ||
|
|
||
| If you're building RPMs or publishing Python packages, you'll want to set up Pulp access in your namespace. It's pretty straightforward. |
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.
The introduction uses a conversational tone (e.g., Think RPMs..., It's pretty straightforward.), which deviates from the professional tone required by the style guide.1 Please revise for a more formal style throughout the document.
Style Guide References
Footnotes
-
Documentation should have a professional tone and good stylistic grammar. ↩
|
|
||
| == Basic setup | ||
|
|
||
| First, you need a TLS certificate and key. Create a secret with them: |
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.
The style guide requires using the imperative mood for user instructions.1 This sentence is descriptive rather than instructive. Please rephrase it as a direct command.
First, create a secret with your TLS certificate and key:
Style Guide References
Footnotes
-
Documentation should use imperative mood language for instructions. ↩
| kubectl get pulpaccessrequest my-pulp-access -o yaml | ||
| ---- | ||
|
|
||
| Look for the status section. If `conditions` shows `Ready: True`, you're good to go. The status also tells you: |
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.
The phrases you're good to go and tells you are too informal for technical documentation.1 Please use more formal language as required by the style guide.
Look for the status section. If `conditions` shows `Ready: True`, the setup is complete. The status also includes:
Style Guide References
Footnotes
-
Documentation should have a professional tone and good stylistic grammar. ↩
|
|
||
| If it prints `True`, everything's set up. | ||
|
|
||
| == Common gotchas |
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.
|
|
||
| == Why use Pulp? | ||
|
|
||
| If you're wondering why we're pushing non-container artifacts to Pulp instead of keeping them local or using random file storage: |
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 sentence is too conversational for technical documentation.1 Please rephrase it to be more direct and professional, for example, by stating the purpose of the section.
Using Pulp provides the following benefits:
Style Guide References
Footnotes
-
Documentation should have a professional tone and good stylistic grammar. ↩
|
|
||
| == What you get | ||
|
|
||
| When you create a `PulpAccessRequest`, the controller sets up: |
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.
Which controller? Is it a konflux controler? Can we elaborate more here?
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 refreshed it, it's the pulp-access-controller, yes it is a konflux controller.
| [source,yaml] | ||
| ---- | ||
| - name: rpmbuild-and-push | ||
| image: quay.io/fedora/fedora:latest |
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.
Neither pulp nor rpmbuild are part of the image, so this example doesn't work.
I'd expect some better integration with RPM build task, not that users must write custom rpmbuild and push task. Will be custom build task even accepted by conforma?
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 meant it as a high-level type of example, configure your pulp using the cert etc, and make use of it. And I wanted to make it simple
But maybe it shouldn't be there at all?
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
A doc describing the pulp-access-controller