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

Remove Framework from Configurator #81374

Open
wants to merge 2 commits into
base: master
from

Conversation

@ahmad-diaa
Copy link
Contributor

commented Aug 13, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes framework from configurator as a step towards removing all other fields from the configurator, in order to make the scheduler independent of the factory.configFactory and Configurator types

Which issue(s) this PR fixes:

ref #72547

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Hi @ahmad-diaa. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ahmad-diaa
To complete the pull request process, please assign bsalamat, mrhohn
You can assign the PR to them by writing /assign @bsalamat @mrhohn in a comment when ready.

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

@ahmad-diaa

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

/assign @alculquicondor

@johnSchnake

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

/ok-to-test

@@ -213,25 +210,17 @@ type ConfigFactoryArgs struct {
PercentageOfNodesToScore int32
BindTimeoutSeconds int64
StopCh <-chan struct{}
Registry framework.Registry
Plugins *config.Plugins
PluginConfig []config.PluginConfig

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

So the only reason for these items in the config was to later create the framework, right?

This comment has been minimized.

Copy link
@ahmad-diaa

ahmad-diaa Aug 14, 2019

Author Contributor

yes, couldn't find them referenced anywhere else

factory := newConfigFactory(client, v1.DefaultHardPodAffinitySymmetricWeight, stopCh)
framework, err := framework.NewFramework(framework.NewRegistry(), nil, []config.PluginConfig{})
if err != nil {
t.Errorf("error initializing the scheduling framework: %v", err)

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

If you can't initialize the framework it seems like you'd want a t.Fatalf instead of just a t.Errorf. Is it the norm that the test will continue and provide meaningful feedback after a failure here?

This comment has been minimized.

Copy link
@johnSchnake

johnSchnake Aug 14, 2019

Contributor

Same comment applies to other similar locations

This comment has been minimized.

Copy link
@ahmad-diaa

ahmad-diaa Aug 14, 2019

Author Contributor

I agree, it makes more sense to use a t.Fatalf instead.

@alculquicondor

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

/hold

I would like some agreement in the issue before considering this PR

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

@ahmad-diaa: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big 8d2f8f4 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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

@johnSchnake

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Seems fine to me from just a basic code perspective but I don't have much context or familiarity with the sections/umbrella issue involved.

I'll put my lgtm but will let others more familiar decide to remove the hold when/if appropriate.

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.