OCPBUGS-99941: replace invalid pip-install input with explicit pip install step - #9133
OCPBUGS-99941: replace invalid pip-install input with explicit pip install step#9133dhgautam99 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
openshift-ci
Bot
added
area/control-plane-operator
…t pip install step The `pip-install` input is no longer a valid input for `actions/setup-python`, causing the docs build to fail with `mkdocs: command not found` across all PRs. Replace it with an explicit `pip install -r docs/requirements.txt` step.
7d9622b to
5ce39c5
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dhgautam99 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 |
dhgautam99
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-99941, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-99941, which is invalid:
Comment 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. |
📝 WalkthroughWalkthroughThe reusable documentation build workflow replaces the previous Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (5)
control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go (1)
93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
.LogLevel != nilover whole-struct comparison for consistency and future-safety.Every sibling component (kcm, kube-scheduler, oapi) checks
X.LogLevel != nil, but this file compares the entireEtcdOperatorSpecstruct against its zero value. It's equivalent today, but inconsistent, and a Go struct equality (!=) will fail to compile if a non-comparable field (slice/map) is ever added toComponentLogLevelSpec/EtcdOperatorSpec.♻️ Proposed fix
if hcp.Spec.OperatorConfiguration != nil && - hcp.Spec.OperatorConfiguration.Etcd != (hyperv1.EtcdOperatorSpec{}) { + hcp.Spec.OperatorConfiguration.Etcd.LogLevel != nil {🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go` around lines 93 - 100, Update the Etcd environment-variable guard in the hosted control plane StatefulSet construction to check hcp.Spec.OperatorConfiguration.Etcd.LogLevel != nil instead of comparing the entire EtcdOperatorSpec with its zero value. Preserve the existing ETCD_LOG_LEVEL assignment and conversion through util.LogLevelToEtcdLevel.control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go (1)
25-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant nil-check duplicated identically across every component's verbosity resolver.
util.LogLevelToKlogVerbosityalready returns the default (2) when passed anil*LogLevel, so the extraX.LogLevel != nilguard in eachresolveXXXVerbosityis dead code, copy-pasted across at least three component packages (likely more, e.g. oauth/oauth_apiserver/ocm, not in this batch).
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go#L25-L32: drop theKubeControllerManager.LogLevel != nilcheck; returnutil.LogLevelToKlogVerbosity(hcp.Spec.OperatorConfiguration.KubeControllerManager.LogLevel)onceOperatorConfiguration != nilis confirmed.control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/deployment.go#L46-L53: same simplification forKubeScheduler.LogLevel.control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go#L124-L131: same simplification forOpenShiftAPIServer.LogLevel.Consider extracting a single shared helper in
support/util(e.g. taking the resolved*hyperv1.OperatorConfigurationand a field-accessor) to prevent this boilerplate from spreading further as more components adopt log-level configuration.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go` around lines 25 - 32, Remove the redundant LogLevel nil checks from resolveKCMVerbosity in control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go:25-32, resolveKubeSchedulerVerbosity in control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/deployment.go:46-53, and resolveOAPIVerbosity in control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go:124-131; after confirming OperatorConfiguration is non-nil, pass each component’s LogLevel directly to util.LogLevelToKlogVerbosity and retain the existing default path. A shared helper is optional and not required for these changes.control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go (1)
1064-1183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest case names don't follow the "When ... it should ..." convention.
These new entries ("Default feature set, KAS Debug log level", etc.) keep the existing table's naming style, but the sibling resolver tests added in this same PR (
kas/deployment_test.go,etcd/etcd_test.go) do follow "When ... it should ..." per repo convention.As per path instructions, "Always use "When ... it should ..." format for describing test cases when creating unit tests" for
**/*_test.go.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go` around lines 1064 - 1183, Rename the new table-driven test cases in the HostedControlPlane test to follow the “When ... it should ...” convention, including the affected KAS, Etcd, KCM, KubeScheduler, OCM, OpenShift API Server, OAuth API Server, and OAuth Server debug log-level cases. Keep each case’s feature set, mutation, and subDirSuffix unchanged.Source: Path instructions
support/util/loglevel.go (1)
7-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the logging protocol constants.
The
2/4/6/8verbosity values and"info"/"debug"strings encode external logging contracts but are currently unnamed. Define constants so future changes do not require interpreting magic values.As per coding guidelines, “Avoid magic numbers — use named constants.”
Proposed refactor
package util +const ( + defaultKlogVerbosity = 2 + debugKlogVerbosity = 4 + traceKlogVerbosity = 6 + traceAllKlogVerbosity = 8 + etcdInfoLogLevel = "info" + etcdDebugLogLevel = "debug" +) +🤖 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 `@support/util/loglevel.go` around lines 7 - 36, Define named constants for the klog verbosity levels 2, 4, 6, and 8, plus the ETCD log levels "info" and "debug", then update LogLevelToKlogVerbosity and LogLevelToEtcdLevel to return those constants instead of literals.Source: Coding guidelines
control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go (1)
220-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the default verbosity across all component resolvers.
The three resolvers duplicate the shared default
2, allowing component behavior to drift if the logging contract changes.
control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go#L220-L221: replace the hardcoded fallback with the shared default.control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go#L131-L132: replace the hardcoded fallback with the shared default.control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go#L103-L104: replace the hardcoded fallback with the shared default.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go` around lines 220 - 221, Replace the hardcoded verbosity fallback in the resolver at control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go:220-221 with the shared default symbol. Apply the same change to the fallback resolvers at control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go:131-132 and control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go:103-104, preserving the existing resolver behavior while centralizing the default.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/deployment.go`:
- Around line 40-41: Update adaptDeployment to remove any existing static
kube-scheduler verbosity flag, including -v=2, from c.Args before appending the
value generated by resolveSchedulerVerbosity(cpContext.HCP), ensuring only the
dynamic verbosity flag remains.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/ocm/deployment.go`:
- Around line 15-19: Add unit-test coverage for adaptDeployment, using a
Deployment containing the OCM container and a configured HCP verbosity level,
then assert the container receives the expected --v=<n> argument after
adaptation. Keep TestResolveOCMVerbosity focused on the resolver and verify the
mutation path through adaptDeployment directly.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go`:
- Around line 1064-1183: Rename the new table-driven test cases in the
HostedControlPlane test to follow the “When ... it should ...” convention,
including the affected KAS, Etcd, KCM, KubeScheduler, OCM, OpenShift API Server,
OAuth API Server, and OAuth Server debug log-level cases. Keep each case’s
feature set, mutation, and subDirSuffix unchanged.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/etcd/statefulset.go`:
- Around line 93-100: Update the Etcd environment-variable guard in the hosted
control plane StatefulSet construction to check
hcp.Spec.OperatorConfiguration.Etcd.LogLevel != nil instead of comparing the
entire EtcdOperatorSpec with its zero value. Preserve the existing
ETCD_LOG_LEVEL assignment and conversion through util.LogLevelToEtcdLevel.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go`:
- Around line 220-221: Replace the hardcoded verbosity fallback in the resolver
at
control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go:220-221
with the shared default symbol. Apply the same change to the fallback resolvers
at
control-plane-operator/controllers/hostedcontrolplane/v2/oauth/deployment.go:131-132
and
control-plane-operator/controllers/hostedcontrolplane/v2/oauth_apiserver/deployment.go:103-104,
preserving the existing resolver behavior while centralizing the default.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go`:
- Around line 25-32: Remove the redundant LogLevel nil checks from
resolveKCMVerbosity in
control-plane-operator/controllers/hostedcontrolplane/v2/kcm/deployment.go:25-32,
resolveKubeSchedulerVerbosity in
control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/deployment.go:46-53,
and resolveOAPIVerbosity in
control-plane-operator/controllers/hostedcontrolplane/v2/oapi/deployment.go:124-131;
after confirming OperatorConfiguration is non-nil, pass each component’s
LogLevel directly to util.LogLevelToKlogVerbosity and retain the existing
default path. A shared helper is optional and not required for these changes.
In `@support/util/loglevel.go`:
- Around line 7-36: Define named constants for the klog verbosity levels 2, 4,
6, and 8, plus the ETCD log levels "info" and "debug", then update
LogLevelToKlogVerbosity and LogLevelToEtcdLevel to return those constants
instead of literals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| c.Args = append(c.Args, fmt.Sprintf("-v=%d", resolveSchedulerVerbosity(cpContext.HCP))) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd deployment.yaml control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-scheduler --exec grep -n -- "-v=" {}Repository: openshift/hypershift
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== asset deployment context =="
fd deployment.yaml control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-scheduler --exec sed -n '20,45p' {} | cat -n
echo
echo "== controller verbosity construction context =="
fd deployment.go control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler --exec sed -n '1,90p' {} | cat -n
echo
echo "== kube-scheduler args in repo =="
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' 'kube-scheduler|Args:|resolveSchedulerVerbosity|-v=' control-plane-operator | head -200Repository: openshift/hypershift
Length of output: 39141
Remove the static kube-scheduler verbosity flag before appending the dynamic one.
control-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-scheduler/deployment.yaml still contains -v=2, so adaptDeployment() appending fmt.Sprintf("-v=%d", resolveSchedulerVerbosity(cpContext.HCP)) can result in duplicate/conflicting verbosity flags on the kube-scheduler container.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/deployment.go`
around lines 40 - 41, Update adaptDeployment to remove any existing static
kube-scheduler verbosity flag, including -v=2, from c.Args before appending the
value generated by resolveSchedulerVerbosity(cpContext.HCP), ensuring only the
dynamic verbosity flag remains.
| func adaptDeployment(cpContext component.WorkloadContext, deployment *appsv1.Deployment) error { | ||
| podspec.UpdateContainer(ComponentName, deployment.Spec.Template.Spec.Containers, func(c *corev1.Container) { | ||
| c.Args = append(c.Args, fmt.Sprintf("--v=%d", resolveOCMVerbosity(cpContext.HCP))) | ||
| }) | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for the Deployment mutation.
TestResolveOCMVerbosity covers the resolver but not this new adaptation path. Add a test that invokes adaptDeployment with a configured level and asserts the OCM container receives the expected --v=<n> argument. As per coding guidelines, “Unit test any code changes and additions.”
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/ocm/deployment.go`
around lines 15 - 19, Add unit-test coverage for adaptDeployment, using a
Deployment containing the OCM container and a configured HCP verbosity level,
then assert the container receives the expected --v=<n> argument after
adaptation. Keep TestResolveOCMVerbosity focused on the resolver and verify the
mutation path through adaptDeployment directly.
Source: Coding guidelines
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
openshift-ci-robot
removed
the
jira/invalid-bug
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-99941, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-99941, which is valid. 3 validation(s) were run on this bug
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. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
I was confused as to how this fixed the issue because your PR's action is still pinned to the old version 6 which still works with the code you're deleting... I realize that it's because you need to rebase from upstream |
The CodeRabbit review even noticed: #9089 (comment) |
|
How do we kick off a test build? I don't see one scheduled even though this change impacts the docs build.... |
|
Thanks for the quick fix on the However, this PR also needs to update the paths:
- 'docs/**'
- '.github/workflows/docs-build.yaml'This is the root cause of how the regression in #9089 slipped through — changes to Could you add the reusable workflow to the paths list? paths:
- 'docs/**'
- '.github/workflows/docs-build.yaml'
- '.github/workflows/docs-build-reusable.yaml'This way:
|
actions/setup-python removed the pip-install input in v7.0.0 (it installed into the global environment, conflicted with virtualenvs and tools like uv, and wasn't picked up by Dependabot). Since this workflow was already pinned to v7.0.0, its pip-install: '-r docs/requirements.txt' input silently became a no-op, meaning docs dependencies were never actually installed here. Replace it with an explicit "pip install -r docs/requirements.txt" step, matching the fix already proposed in openshift#9133. Also watch docs-build-reusable.yaml in docs-build.yaml's paths filter. That filter only watched docs/** and its own file, not the reusable workflow it calls, which is how this exact breakage (from PR openshift#9089's actions/setup-python bump) slipped through unnoticed: the Docs Build check never ran on that PR since the file it changed wasn't in the filter. Every other trigger/-reusable workflow pair in this repo already references its reusable file in the paths filter; this was an isolated gap specific to docs-build.yaml.
actions/setup-python removed the pip-install input in v7.0.0 (it installed into the global environment, conflicted with virtualenvs and tools like uv, and wasn't picked up by Dependabot). Since this workflow was already pinned to v7.0.0, its pip-install: '-r docs/requirements.txt' input silently became a no-op, meaning docs dependencies were never actually installed here. Replace it with an explicit "pip install -r docs/requirements.txt" step, matching the fix already proposed in openshift#9133.
openshift-ci
Bot
added
the
needs-rebase
|
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. |
|
Fix included in #9135 , Thanks @dhgautam99 ! /close |
|
@mgencur: Closed this PR. 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 kubernetes-sigs/prow repository. |
|
@dhgautam99: This pull request references Jira Issue OCPBUGS-99941. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. 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. |
actions/setup-python removed the pip-install input in v7.0.0 (it installed into the global environment, conflicted with virtualenvs and tools like uv, and wasn't picked up by Dependabot). Since this workflow was already pinned to v7.0.0, its pip-install: '-r docs/requirements.txt' input silently became a no-op, meaning docs dependencies were never actually installed here. Replace it with an explicit "pip install -r docs/requirements.txt" step, matching the fix already proposed in openshift#9133.
actions/setup-python removed the pip-install input in v7.0.0 (it installed into the global environment, conflicted with virtualenvs and tools like uv, and wasn't picked up by Dependabot). Since this workflow was already pinned to v7.0.0, its pip-install: '-r docs/requirements.txt' input silently became a no-op, meaning docs dependencies were never actually installed here. Replace it with an explicit "pip install -r docs/requirements.txt" step, matching the fix already proposed in openshift#9133.
7 participants
What this PR does / why we need it:
The
pip-installinput is no longer a valid input foractions/setup-python, causing the "Build Docs" CI job to fail withmkdocs: command not foundon all PRs. This replaces the invalid input with an explicitpip install -r docs/requirements.txtstep.Which issue(s) this PR fixes:
Fixes OCPBUGS-99941
Special notes for your reviewer:
This is a repo-wide CI fix — the "Build Docs" job is currently broken for new PRs (e.g., #9132 and #8878). The
pip-installinput was introduced in PR #8386 and worked initially, butactions/setup-pythonhas since removed it as a valid input.Checklist:
Summary by CodeRabbit
docs/requirements.txtfile consistently.