NO-JIRA: performance profile: internal cleanup #1359
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest-required |
1 similar comment
|
/retest-required |
reorganize the current helper.go dissolving it into specific subpackages. The goal is tstreamline the code layout without changes in behavior. Signed-off-by: Francesco Romani <[email protected]>
the `cmd/pkg` layout is awkward. Canonically: - code used only by a tool should sit in `internal/` - code which is/can be exported should sit in `pkg`. However, in case of nontrivial command line utilities, it's a widespread practice to have `pkg/cmd` (or `pkg/commands`) trees. This enable third party utilities to easily embed other utilities maximizing code reuse. We streamline the code layout with trivial code movements - no intended changes of behavior. The net result is now `cmd/performance-profile-creator` is now minified, and holds only the entry point source code file. Signed-off-by: Francesco Romani <[email protected]>
extract code which deals with mustgather or ghw in their own source files. Trivial code movement with no changes in behavior. Signed-off-by: Francesco Romani <[email protected]>
- add pure text output command. By default (and it's very hard to change, if we ever should) log messages go on stderr. We want the output on stdout. Added flag `--text` to enable it - internal cleanup and code reorganization. In log mode, log all the message in one go, which is easier to manage. No intended change in behavior. Signed-off-by: Francesco Romani <[email protected]>
We used the `logrus` package internally, but minimally and pretty much only to notify processing status to the user. This code was thus the only remaining consumer of that package, which we don't really need. Add a neutral (nor log, nor warning) minimal abstraction `Alert`, similar in spirit to `ghw`, to be used when we need to notify the user about processing status. Make the backend easy to replace to make integration of this code easier. We don't see this happening, but the extra cost is negligible. Remove the direct dependency to `logrus`. Note we still have quite many indirect dependencies pulling it in. Signed-off-by: Francesco Romani <[email protected]>
ffromani
force-pushed
the
perfprof-creator-cleanup
branch
from
310c413 to
405c966
Compare
|
/retest-required |
|
/test okd-scos-e2e-aws-ovn |
|
@ffromani: all tests passed! 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. |
|
Thank you for the PR. Any vendored dependency removal is welcome. The changes look fine to me, even at the expense of issuing an extra "\n" in /lgtm |
Thanks Jiri! Be aware that my #1349 wants to add back another dependency though. But the balance should still be positive. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
ffromani
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ffromani: This pull request explicitly references no jira issue. In 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. |
ffromani
changed the title
|
/hold cancel I'll own and fix any regression. I asked previously and the output text format is not part of the API/stability guarantee, so we should be good |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[ART PR BUILD NOTIFIER] Distgit: cluster-node-tuning-operator |
internal cleanups, code movement and minimal refactoring in preparation for the autosizing feature (#1349)
No intended changes in behavior.
The switch to the
Alertmechanism is mostly meant to remove our remaining dependency fromlogrus.