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.

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

feat(scheduler): rename PluginContext to CycleState #83430

Conversation

draveness
Copy link
Contributor

@draveness draveness commented Oct 3, 2019

/kind cleanup
/sig scheduling
/assign @alculquicondor @ahg-g

What this PR does / why we need it:
rename PluginContext to CycleState

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Rename PluginContext to CycleState in the scheduling framework

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 3, 2019
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 3, 2019
@draveness
Copy link
Contributor Author

/retest

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Any particular reason why you used the variable name cycleState in some places and state in others?
I'm ok with just having state for the variables unless there is ambiguity.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2019
@draveness
Copy link
Contributor Author

Any particular reason why you used the variable name cycleState in some places and state in others?
I'm ok with just having state for the variables unless there is ambiguity.

I make the following changes

  • pluginContext -> cycleState
  • pc -> state

@alculquicondor
Copy link
Member

To increase consistency, I suggest we just use state unless there's a collision. WDYT @ahg-g ?

@draveness
Copy link
Contributor Author

draveness commented Oct 3, 2019

To increase consistency, I suggest we just use state unless there's a collision. WDYT @ahg-g ?

I think it would be hard to do this since we already have such inconsistency among the codebase, e.g. pluginContext and pc. If you think it would be a problem, I could replace cycleState with state in the PR. But it does not sound like a big problem to me.

@ahg-g
Copy link
Member

ahg-g commented Oct 3, 2019

To increase consistency, I suggest we just use state unless there's a collision. WDYT @ahg-g ?

+1 for consistency. We can't use cs since it refers usually to a ClientSet instance, so I guess we can set the precedence that state in the context of the scheduler means CycleState.

@draveness draveness force-pushed the feature/rename-plugincontext-to-cyclestate branch from 4f29308 to 40df91e Compare October 3, 2019 15:25
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2019
@draveness
Copy link
Contributor Author

To increase consistency, I suggest we just use state unless there's a collision. WDYT @ahg-g ?

+1 for consistency. We can't use cs since it refers usually to a ClientSet instance, so I guess we can set the precedence that state in the context of the scheduler means CycleState.

done

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks @draveness

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2019
@ahg-g
Copy link
Member

ahg-g commented Oct 3, 2019

/approve

@ahg-g
Copy link
Member

ahg-g commented Oct 3, 2019

/test pull-kubernetes-e2e-gce

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, draveness

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2019
@draveness
Copy link
Contributor Author

/retest

@ahg-g
Copy link
Member

ahg-g commented Oct 4, 2019

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 4, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@draveness
Copy link
Contributor Author

draveness commented Oct 4, 2019 via email

@draveness draveness force-pushed the feature/rename-plugincontext-to-cyclestate branch from 40df91e to c73ff97 Compare October 4, 2019 09:30
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019
@draveness
Copy link
Contributor Author

draveness commented Oct 4, 2019

Hi, @ahg-g. I just rebased the master and fixed the test, PTAL

@ahg-g
Copy link
Member

ahg-g commented Oct 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2019
@draveness
Copy link
Contributor Author

draveness commented Oct 4, 2019 via email

@k8s-ci-robot k8s-ci-robot merged commit eba349b into kubernetes:master Oct 4, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Oct 4, 2019
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. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants