Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: KSM Cyclomatic fix for Job and PV files #2092

Closed
wants to merge 9 commits into from
Closed

refactor: KSM Cyclomatic fix for Job and PV files #2092

wants to merge 9 commits into from

Conversation

venkatbvc
Copy link

@venkatbvc venkatbvc commented Jun 7, 2023 •
edited
Loading

Fixed cyclomatic complexity for Job.go and persistentVolume.go files
Changes to be committed:
modified: internal/store/job.go
modified: internal/store/persistentvolume.go

What this PR does / why we need it:
Fix the cyclomatic complexity for job.go and persistentVolume.go
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
No change to cardinality
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
Fixes partially 1887

@chalapat
KSM Cyclomatic fix for Job and PV files
d382720 
 Changes to be committed:
	modified:   internal/store/job.go
	modified:   internal/store/persistentvolume.go
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2023
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 7, 2023
@venkatbvc
Copy link
Author

@rexagod Pushed changes for Job.go and persistentVolume.go. Please review

@venkatbvc
Copy link
Author

/cc @rexagod

@k8s-ci-robot k8s-ci-robot requested a review from rexagod June 7, 2023 15:02
@venkatbvc
Copy link
Author

@rexagod @CatherineF-dev @fpetkovski can you please review and share your comments

@CatherineF-dev
Copy link
Contributor

Reviewed internal/store/job.go.

If you split this PR into two PRs (job and pv), I think the job one can be merged asap.

chalapat and others added 4 commits June 14, 2023 22:25
@chalapat
KSM Cyclomatic fix for Job and PV files
37cbe28 
reverting the Persistent volume changes
 Changes to be committed:
	modified:   internal/store/job.go
	modified:   internal/store/persistentvolume.go
@venkatbvc
Merge branch 'gocylo_incremental_fix' of https://github.com/venkatbvc…
ecf361a 
…/kube-state-metrics into gocylo_incremental_fix
@venkatbvc @chalapat
Fix KSM cyclomatic complexity for job.go
f473c70
@venkatbvc
Merge branch 'gocylo_incremental_fix' of https://github.com/venkatbvc…
a5f19fd 
…/kube-state-metrics into gocylo_incremental_fix
@venkatbvc venkatbvc changed the title KSM Cyclomatic fix for Job and PV files Fix- KSM Cyclomatic fix for Job and PV files Jun 14, 2023
@venkatbvc venkatbvc changed the title Fix- KSM Cyclomatic fix for Job and PV files fix- KSM Cyclomatic fix for Job and PV files Jun 14, 2023
@venkatbvc venkatbvc changed the title fix- KSM Cyclomatic fix for Job and PV files fix KSM Cyclomatic fix for Job and PV files Jun 14, 2023
@venkatbvc venkatbvc changed the title fix KSM Cyclomatic fix for Job and PV files refactor KSM Cyclomatic fix for Job and PV files Jun 14, 2023
@venkatbvc venkatbvc changed the title refactor KSM Cyclomatic fix for Job and PV files refactor: KSM Cyclomatic fix for Job and PV files Jun 14, 2023
@venkatbvc
Copy link
Author

@CatherineF-dev I reverted the pv changes. Please check.

venkatbvc added 2 commits June 15, 2023 20:45
@venkatbvc @chalapat
refactor: revert changes to persistentvolume
862bee0 
 Changes to be committed:
	modified:   internal/store/persistentvolume.go
@venkatbvc
Merge branch 'gocylo_incremental_fix' of https://github.com/venkatbvc…
84d7503 
…/kube-state-metrics into gocylo_incremental_fix
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 15, 2023
@logicalhan
Copy link
Member

/triage accepted
/assign @CatherineF-dev

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jun 15, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jun 15, 2023
@logicalhan
Copy link
Member

/assign @rexagod

@venkatbvc
Copy link
Author

@rexagod @CatherineF-dev Can you please look at the changes?

@CatherineF-dev
Copy link
Contributor

/lgtm

Waiting @rexagod's review

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CatherineF-dev, venkatbvc
Once this PR has been reviewed and has the lgtm label, please ask for approval from rexagod. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@venkatbvc
Copy link
Author

@rexagod can you please review?

@rexagod
Copy link
Member

rexagod commented Jun 23, 2023

Hello, @venkatbvc. Thank you for the PR, however, there's some development recently that affects the areas covered by the parent issue. With the CEL and CRD changes planned for the near future, we are again going to need to check for any lint (warn-level, that do not block the CI) issues again once those changes are in.

I think it's better to stall this for now and resume this effort once that is done with to avoid repeating the same effort. In the meantime, if you're looking for a good-first-issue we've got this: #1903 that's been there for a while, and I'd be happy to help with anything around it! 🙂

@rexagod
Copy link
Member

rexagod commented Jun 23, 2023

/hold
Until aforementioned factors clear up.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2023
@venkatbvc
Copy link
Author

@rexagod Sure we will wait. This week i am little busy. i will get in touch with you next week

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2023
@k8s-ci-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/test-infra repository.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 21, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 20, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CatherineF-dev CatherineF-dev Awaiting requested review from CatherineF-dev

@fpetkovski fpetkovski Awaiting requested review from fpetkovski

@rexagod rexagod Awaiting requested review from rexagod

Assignees

@rexagod rexagod

@CatherineF-dev CatherineF-dev

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@venkatbvc @CatherineF-dev @logicalhan @k8s-ci-robot @rexagod @k8s-triage-robot @chalapat