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

fix openapi handling when used in conjunction with deployment.container-image=true #350

Merged
merged 2 commits into from Jan 21, 2019

Conversation

lburgazzoli
Copy link
Contributor

No description provided.

Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

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

Looks good! Just a minor comment if you want to fix it

if gBuilder != nil {
return gBuilder, nil
}

gBuilder = builder.New(ctx, c, namespace)
gBuilder = builder.New(context.TODO(), c, namespace)
Copy link
Member

Choose a reason for hiding this comment

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

Why creating a temp one and not propagating the existing one? You can pass a context.TODO if a context is not available at caller site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that the builder is created once per platform so if we use the context of the caller, it will be the one of the first caller and it can be closed which causes the builder to stop.

I need to think about a better solution but in a separate pr

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Jan 21, 2019

@nicolaferraro

I've polished a little bit action handlers, some new context.TODO() are needed as looking at the code, a context is bound to a single reconcile loop so for things that may happen asynchronously we need a dedicated context.

I need to check how this can be improved but will take a little time

@nicolaferraro nicolaferraro merged commit a588b45 into apache:master Jan 21, 2019
@lburgazzoli lburgazzoli deleted the container-image branch January 21, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants