ART-20930: add golang-builder-shipment pipeline#3083
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new ChangesGolang Builder Shipment Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 72-83: Add validation for the golang_group parameter in the
__init__ method before it is assigned to self.golang_group. Implement an
allow-list approach to reject any golang_group values containing path traversal
patterns like forward slashes or double dots (..). This validation at the trust
boundary will prevent malicious payloads from escaping the intended directory
layout when golang_group is subsequently used in branch name construction and
filesystem path operations.
- Around line 126-129: The basic_auth_url function embeds the GITLAB_TOKEN
directly into the Git remote URL, which persists credentials in .git/config and
increases accidental leakage risk. Instead of injecting the token into the URL
via the basic_auth_url function call at lines 161-163, refactor the code to use
git credential helpers or pass authentication via HTTP headers. Remove or
refactor the basic_auth_url function and configure git operations to
authenticate using environment variables (like GIT_CREDENTIALS) or by passing
authentication headers directly in API calls rather than embedding credentials
in the remote URL itself.
- Around line 199-200: Remove the `f` prefix from the static string literals at
lines 199-200 in the golang_builder_shipment.py file. The strings "The golang
builder images are available from " and
"registry.redhat.io/openshift/golang-builder." do not contain any placeholder
expressions (curly braces with variables), so they should be regular strings
without the `f` prefix to comply with Ruff linting rule F541.
- Line 129: The `basic_auth_url` construction uses `parsed.hostname` which
strips the port number from URLs, breaking access to custom GitLab endpoints
with non-default ports. Replace `parsed.hostname` with `parsed.netloc` in the
return statement on line 129 to preserve the full host:port combination from the
parsed URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4cbbfe7c-d8b0-43e8-a06c-29e23bddb20b
📒 Files selected for processing (4)
pyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/__init__.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
| golang_group: str, | ||
| env: str, | ||
| nvrs: List[str], | ||
| art_jira: str = "", | ||
| shipment_data_repo_url: Optional[str] = None, | ||
| ): | ||
| self.runtime = runtime | ||
| self.ocp_version = ocp_version | ||
| self.golang_group = golang_group | ||
| self.env = env | ||
| self.nvrs = sorted(nvrs) | ||
| self.art_jira = art_jira |
There was a problem hiding this comment.
Validate golang_group before using it in branch and filesystem paths.
golang_group from CLI flows into Line 264 branch name and Line 269 path construction without allow-list validation. Payloads containing / or .. can escape expected layout and write outside the intended subtree.
Suggested fix
+import re
@@
class GolangBuilderShipmentPipeline:
@@
def __init__(
@@
- self.golang_group = golang_group
+ if not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9._-]*", golang_group):
+ raise ValueError(f"Invalid golang_group: {golang_group!r}")
+ self.golang_group = golang_groupAs per coding guidelines: “Path traversal: canonicalize paths, reject ../” and “Validate at trust boundaries with allow-lists, not deny-lists”.
Also applies to: 264-270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py` around lines 72 - 83,
Add validation for the golang_group parameter in the __init__ method before it
is assigned to self.golang_group. Implement an allow-list approach to reject any
golang_group values containing path traversal patterns like forward slashes or
double dots (..). This validation at the trust boundary will prevent malicious
payloads from escaping the intended directory layout when golang_group is
subsequently used in branch name construction and filesystem path operations.
Source: Coding guidelines
| def basic_auth_url(url: str, token: str) -> str: | ||
| """Inject token into a GitLab URL for push authentication.""" | ||
| parsed = urlparse(url) | ||
| return f"{parsed.scheme}://oauth2:{token}@{parsed.hostname}{parsed.path}" |
There was a problem hiding this comment.
Preserve explicit port in basic_auth_url.
Line 129 uses parsed.hostname, which drops non-default ports from input URLs (e.g., :8443) and can break repo access for custom GitLab endpoints.
Suggested fix
def basic_auth_url(url: str, token: str) -> str:
"""Inject token into a GitLab URL for push authentication."""
parsed = urlparse(url)
- return f"{parsed.scheme}://oauth2:{token}@{parsed.hostname}{parsed.path}"
+ host = parsed.hostname if parsed.port is None else f"{parsed.hostname}:{parsed.port}"
+ return f"{parsed.scheme}://oauth2:{token}@{host}{parsed.path}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py` at line 129, The
`basic_auth_url` construction uses `parsed.hostname` which strips the port
number from URLs, breaking access to custom GitLab endpoints with non-default
ports. Replace `parsed.hostname` with `parsed.netloc` in the return statement on
line 129 to preserve the full host:port combination from the parsed URL.
lgarciaaco
force-pushed
the
art-20930/golang-builder-shipment
branch
2 times, most recently
from
aa4906d to
fe664c4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyartcd/pyartcd/pipelines/golang_builder_shipment.py (1)
311-311: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAccessing private
_directoryattribute ofGitRepository.Line 311 directly accesses
self.shipment_data_repo._directory, which is a private attribute (prefixed with_). Consider exposing a public property or method inGitRepositoryto access the directory path, or use the existing public interface if available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py` at line 311, The code at line 311 in golang_builder_shipment.py directly accesses the private attribute `_directory` on the `shipment_data_repo` GitRepository object. Replace this direct access to the private attribute with a call to an existing public method or property of the GitRepository class that returns the directory path. If no public interface exists for this, add a public property method to the GitRepository class (such as `get_directory()` or `directory`) that returns the path, then use that public interface instead of accessing `_directory` directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Line 311: The code at line 311 in golang_builder_shipment.py directly accesses
the private attribute `_directory` on the `shipment_data_repo` GitRepository
object. Replace this direct access to the private attribute with a call to an
existing public method or property of the GitRepository class that returns the
directory path. If no public interface exists for this, add a public property
method to the GitRepository class (such as `get_directory()` or `directory`)
that returns the path, then use that public interface instead of accessing
`_directory` directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5b05c86-1b25-44ff-82ad-b4f458a3a47e
📒 Files selected for processing (4)
pyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/__init__.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
✅ Files skipped from review due to trivial changes (1)
- pyartcd/pyartcd/pipelines/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/pyartcd/main.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Line 24: Remove the unused `Dict` type import from the import statement on
line 24 in the typing imports. The `Dict` type hint is imported but never used
anywhere in the file, which causes the Ruff F401 linting rule to fail. Simply
remove `Dict` from the comma-separated list of imports while keeping `List`,
`Optional`, and `Tuple` which are actually used in the file.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py`:
- Line 231: The assignment of `_gitlab_token` to the hardcoded string literal
"test-token" violates security guidelines for handling sensitive values. Replace
the hardcoded string literal with a dynamically generated placeholder using
secrets.token_hex() or a similar method to generate a random token string,
ensuring the test maintains functionality while avoiding secret-pattern
violations and adhering to the coding guidelines for variables named token,
secret, api_key, or password.
- Around line 1-4: The import ordering in the test file is not following
alphabetical order as required by Ruff's I001 rule. Reorganize the imports at
the top of the file so that standard library imports are sorted alphabetically
by module name. The pathlib import should come before the unittest imports since
"pathlib" comes before "unittest" alphabetically. Group related imports from the
same module together after sorting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b83b366-27ae-424c-95d5-0244d840ff43
📒 Files selected for processing (4)
pyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/__init__.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyartcd/pyartcd/pipelines/init.py
- pyartcd/pyartcd/main.py
| import unittest | ||
| from unittest import IsolatedAsyncioTestCase | ||
| from unittest.mock import AsyncMock, MagicMock, Mock, patch | ||
| from pathlib import Path |
There was a problem hiding this comment.
Fix import ordering to unblock Ruff.
Line 1–Line 4 import order is failing ruff (I001) in CI.
Suggested diff
-import unittest
-from unittest import IsolatedAsyncioTestCase
-from unittest.mock import AsyncMock, MagicMock, Mock, patch
from pathlib import Path
+import unittest
+from unittest import IsolatedAsyncioTestCase
+from unittest.mock import AsyncMock, MagicMock, Mock, patch🧰 Tools
🪛 GitHub Actions: unit-tests / 0_tests.txt
[error] 1-1: ruff check (I001): Import block is un-sorted or un-formatted
🪛 GitHub Actions: unit-tests / tests
[error] 1-1: ruff check failed (Ruff rule I001): Import block is un-sorted or un-formatted
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py` around lines 1 - 4,
The import ordering in the test file is not following alphabetical order as
required by Ruff's I001 rule. Reorganize the imports at the top of the file so
that standard library imports are sorted alphabetically by module name. The
pathlib import should come before the unittest imports since "pathlib" comes
before "unittest" alphabetically. Group related imports from the same module
together after sorting.
Source: Pipeline failures
| ) | ||
| pipeline.env = "prod" | ||
| pipeline.release_plan = "ocp-art-golang-builder-prod-rhel9" | ||
| pipeline._gitlab_token = "test-token" |
There was a problem hiding this comment.
Avoid hardcoded token literals, even in tests.
Line 231 assigns _gitlab_token to a string literal. Use a generated placeholder (for example via secrets.token_hex) to avoid secret-pattern violations/noise.
As per coding guidelines, "**/*.{js,ts,tsx,jsx,py,go,java,cs,rb,php,conf,config,yml,yaml,json,toml,properties,env,env.*,.env*,xml,gradle,maven}: Flag hardcoded secrets ... and variables named api_key, secret, token, or password assigned to string literals."
🧰 Tools
🪛 Ruff (0.15.17)
[error] 231-231: Possible hardcoded password assigned to: "_gitlab_token"
(S105)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py` at line 231, The
assignment of `_gitlab_token` to the hardcoded string literal "test-token"
violates security guidelines for handling sensitive values. Replace the
hardcoded string literal with a dynamically generated placeholder using
secrets.token_hex() or a similar method to generate a random token string,
ensuring the test maintains functionality while avoiding secret-pattern
violations and adhering to the coding guidelines for variables named token,
secret, api_key, or password.
Sources: Coding guidelines, Linters/SAST tools
lgarciaaco
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@lgarciaaco: This pull request references ART-20930 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
New artcd command to create shipment MRs in ocp-shipment-data for golang builder images. Auto-resolves Konflux image NVRs from golang RPM NVRs, detects prod vs ec lifecycle from ocp-build-data, and opens a draft MR for ERT approval. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
lgarciaaco
force-pushed
the
art-20930/golang-builder-shipment
branch
from
c863f38 to
4560d5b
Compare
|
@lgarciaaco: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-rebase
2 participants
Summary
New
artcd golang-builder-shipmentpipeline that creates shipment MRs inocp-shipment-datafor golang builder images. This moves golang builder releases from the silent auto-release path to a shipment-gated pipeline requiring ERT approval.GolangBuilderShipmentPipelineclass with lifecycle-to-ReleasePlan mapping (prod→ocp-art-golang-builder-prod-rhel9,ec→ocp-art-golang-builder-ec-rhel9)ShipmentConfigYAML usingelliott snapshot new --builds-fileand theelliottlib.shipment_modelPydantic modelsshipment/ocp/<golang-group>/art-images-base/<env>/stream.image.<timestamp>.yamlgolang-builder-shipmentCLI command in__main__.pyandpipelines/__init__.py--builds-filecommand assemblyTest plan
python -m pytest pyartcd/tests/pipelines/test_golang_builder_shipment.py -vartcd --dry-run golang-builder-shipment --ocp-version 4.22 --golang-group rhel-9-golang-1.25 --env prod NVR1Ref: ART-20930
Summary by CodeRabbit
golang-builder-shipmentcommand to generate shipment configuration from provided Go builder NVRs and open draft merge requests targetingmain.