Skip to content

Conversation

@YasenT
Copy link

@YasenT YasenT commented Nov 24, 2025

A doc describing the pulp-access-controller

@YasenT YasenT requested a review from a team as a code owner November 24, 2025 14:16
Copy link

@gemini-code-assist gemini-code-assist bot left a 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.

Comment on lines +69 to +95
[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
----

Choose a reason for hiding this comment

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

high

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.

Comment on lines 3 to 5
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.

Choose a reason for hiding this comment

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

medium

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

  1. 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:

Choose a reason for hiding this comment

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

medium

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

  1. 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:

Choose a reason for hiding this comment

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

medium

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

  1. Documentation should have a professional tone and good stylistic grammar.


If it prints `True`, everything's set up.

== Common gotchas

Choose a reason for hiding this comment

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

medium

The term gotchas is informal jargon.1 Please use a more professional and descriptive heading, such as Common issues or Troubleshooting.

== Common issues

Style Guide References

Footnotes

  1. Documentation should have a professional tone and good stylistic grammar.


== 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:

Choose a reason for hiding this comment

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

medium

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

  1. Documentation should have a professional tone and good stylistic grammar.


== What you get

When you create a `PulpAccessRequest`, the controller sets up:
Copy link
Contributor

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?

Copy link
Author

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
Copy link
Contributor

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?

Copy link
Author

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-io
Copy link

snyk-io bot commented Nov 27, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@YasenT YasenT requested a review from MartinBasti December 1, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants