Skip to content

Conversation

@deepsm007
Copy link
Contributor

/cc @openshift/test-platform

@deepsm007
Copy link
Contributor Author

/ai pr_description

@openshift-ci openshift-ci bot requested a review from a team October 21, 2025 16:13
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 21, 2025

@deepsm007:
General feedback
You did a great job tightening the error‐handling, adding defer‑closures, and improving readability. The changes are mostly safety‑first – no breaking API changes, just more graceful cleanup and better diagnostics.

What to watch

Location Issue Why it matters Quick fix
cmd/gsm-secret-sync/main.go errors.As(err, &exitErr) used but errors not imported Compile error, test failures Add import "errors"
cmd/autoconfigbrancher/main.go Mismatch in fmt.Errorf formatting (%w\n vs \n) Minor, but keep consistent style Standardize to \n only
.golangci.yml Removed - gosimple, - gofmt linters Might silently miss simple optimisations Re‑enable or add custom exclusion rules
pkg/steps/... Several fmt.Fprintf return checks now use logrus Good, but some places still swallow errors (e.g., single fmt.Fprintf without error return check) Ensure all fmt.Fprintf results are checked (then logged)
pkg/api/constant.go Spelling correction (-creator) OK, but accompany with README note so downstream docs match No action, but keep changelog
pkg/steps/artifacts.go We added logrus.WithError but the package didn’t import logrus in that file Compile error Add import "github.com/sirupsen/logrus"
cmd/pipeline-controller/config_watcher.go Using logrus but not imported ✅ It imports earlier? Yes, check if any missing

Technical notes

  • The use of errors.As is now pervasive and correct. Just double‑check every place you added it – nil panic can happen if you forget the import.
  • The new defer‑closures are consistent, but remember that wrapping os.RemoveAll inside a defer is fine only for temporary directories; for persistent ones it may mask real errors later. This is fine for tests but consider using t.Cleanup where possible (you already use it).
  • The github wrapper now logs body‑closes, which is great for tight loops that could leak files.
  • In pkg/results/error.go you added aggregate error support; pending tests may need to adapt to the new behaviour.

Quick win
Run go vet ./... and golangci-lint run on a minimal subset to catch the missing imports and any unused variables introduced by these patches.

Overall solid work – keep the same style going for the remaining modules, and the repo will be more robust and easier to reason about.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2025
@hector-vido
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2025
@openshift-ci-robot
Copy link
Contributor

Scheduling required tests:
/test e2e

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test integration-optional-test

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 5ce4ecb and 2 for PR HEAD c9243c4 in total

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2025
@deepsm007 deepsm007 changed the title Golangci-lint update to 1.24 and migration from verion 1 -> 2 WIP: Golangci-lint update to 1.24 and migration from verion 1 -> 2 Oct 21, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2025
@hector-vido
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2025
@openshift-ci-robot
Copy link
Contributor

Scheduling required tests:
/test e2e

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test integration-optional-test

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 22, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deepsm007, hector-vido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [deepsm007,hector-vido]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 22, 2025

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 23, 2025

@deepsm007: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e 74a214d link true /test e2e
ci/prow/images 4f70f3a link true /test images
ci/prow/lint 4f70f3a link true /test lint
ci/prow/breaking-changes 4f70f3a link false /test breaking-changes
ci/prow/unit 4f70f3a link true /test unit

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2025
@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

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.

@deepsm007 deepsm007 closed this Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants