-
Notifications
You must be signed in to change notification settings - Fork 152
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
Propagate context to child function #1036
Conversation
pkg/controller/controller_test.go
Outdated
@@ -437,7 +437,7 @@ func (s *ControllerSuite) TestExecActionSet(c *C) { | |||
|
|||
func (s *ControllerSuite) TestRuntimeObjEventLogs(c *C) { | |||
c.Skip("This may not work in MiniKube") | |||
ctx := context.TODO() | |||
contxt := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contxt := context.TODO() | |
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viveksinghggits I don't think we should do that, ctx
is being initialized later in the function with context.Background
in line no 468.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so why don't we use this one there as well.
pkg/controller/controller.go
Outdated
@@ -122,13 +122,14 @@ func (c *Controller) StartWatch(ctx context.Context, namespace string) error { | |||
} | |||
|
|||
func checkCRAccess(cli versioned.Interface, ns string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to pass in context from StartWatch
pkg/controller/controller.go
Outdated
@@ -190,15 +191,16 @@ func (c *Controller) onDelete(obj interface{}) { | |||
} | |||
|
|||
func (c *Controller) onAddActionSet(as *crv1alpha1.ActionSet) error { | |||
as, err := c.crClient.CrV1alpha1().ActionSets(as.GetNamespace()).Get(context.TODO(), as.GetName(), v1.GetOptions{}) | |||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not adding much value. No need to change this function until we handle the ActionSet context stuff you are working on in parallel.
pkg/testing/integration_test.go
Outdated
@@ -304,15 +304,16 @@ func newActionSet(bpName, profile, profileNs string, object crv1alpha1.ObjectRef | |||
} | |||
|
|||
func (s *IntegrationSuite) createProfile(c *C) string { | |||
secret, err := s.cli.CoreV1().Secrets(kontroller.namespace).Create(context.TODO(), s.profile.secret, metav1.CreateOptions{}) | |||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass ctx
in from TestRun
pkg/testutil/testutil_test.go
Outdated
@@ -84,17 +84,18 @@ func (s *TestUtilSuite) TestDeployment(c *C) { | |||
} | |||
|
|||
cm := NewTestConfigMap() | |||
cm, err = cli.CoreV1().ConfigMaps(ns.GetName()).Create(context.TODO(), cm, metav1.CreateOptions{}) | |||
cm, err = cli.CoreV1().ConfigMaps(ns.GetName()).Create(ctx, cm, metav1.CreateOptions{}) | |||
c.Assert(err, IsNil) | |||
err = cli.CoreV1().ConfigMaps(ns.GetName()).Delete(context.TODO(), cm.GetName(), metav1.DeleteOptions{}) | |||
c.Assert(err, IsNil) | |||
} | |||
|
|||
func testCRs(c *C, cli crclientv1alpha1.CrV1alpha1Interface, namespace, poKind, poName string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass it in from above
func testCRs(c *C, cli crclientv1alpha1.CrV1alpha1Interface, namespace, poKind, poName string) { | |
func testCRs(c *C, ctx context.Context, cli crclientv1alpha1.CrV1alpha1Interface, namespace, poKind, poName string) { |
pkg/testutil/testutil_test.go
Outdated
c.Assert(err, IsNil) | ||
err = cli.CoreV1().ConfigMaps(ns.GetName()).Delete(context.TODO(), cm.GetName(), metav1.DeleteOptions{}) | ||
c.Assert(err, IsNil) | ||
} | ||
|
||
func testCRs(c *C, cli crclientv1alpha1.CrV1alpha1Interface, namespace, poKind, poName string) { | ||
var err error | ||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx := context.TODO() |
Co-authored-by: Pavan Navarathna <pavan@kasten.io>
Co-authored-by: Pavan Navarathna <pavan@kasten.io>
Co-authored-by: Pavan Navarathna <pavan@kasten.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Change Overview
This PR is to refactor code and pass the same context object if it’s available in parent call while performing
Create
,List
,Get
operation.Referred PR #726
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan