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: Fix a couple of bugs for ClusterServingRuntime #257

Merged
merged 5 commits into from
Oct 6, 2022

Conversation

chinhuang007
Copy link
Contributor

Fix a couple of bugs for ClusterServingRuntime and also rename two variables to reflect the cluster scope mode is enabled

Signed-off-by: Chin Huang [email protected]

Motivation

Fix bugs and make code more readable

Fix a couple of bugs for ClusterServingRuntime and also rename
two variables to reflect the cluster scope mode is enabled

Signed-off-by: Chin Huang <[email protected]>
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chinhuang007, just a couple of comments

controllers/servingruntime_controller.go Show resolved Hide resolved
controllers/servingruntime_controller.go Outdated Show resolved Hide resolved
controllers/servingruntime_controller.go Outdated Show resolved Hide resolved
controllers/servingruntime_controller.go Outdated Show resolved Hide resolved
@njhill njhill changed the title Fix a couple of bugs for ClusterServingRuntime fix: Fix a couple of bugs for ClusterServingRuntime Oct 5, 2022
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chinhuang007 looks great, just one more small thing I just thought of.

controllers/servingruntime_controller.go Show resolved Hide resolved
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chinhuang007, LGTM now, I guess we just need to track down how this nil deref is happening...

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chinhuang007, latest fix looks good, but can just always set the provided namespace.

controllers/config/manifest.go Outdated Show resolved Hide resolved
controllers/servingruntime_controller.go Outdated Show resolved Hide resolved
@njhill
Copy link
Member

njhill commented Oct 6, 2022

@chinhuang007 incidentally I also noticed that setting a cluster-scoped owner here does nothing: https://github.com/manifestival/manifestival/blob/master/transform.go#L97-L100

which is unfortunate. I wonder whether that statement about it being broken is still true (it's pretty old). The Kube docs don't mention anything about it.

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @chinhuang007, LGTM!

@chinhuang007
Copy link
Contributor Author

It seems a bit odd that doc says " Namespaced dependents can specify cluster-scoped or namespaced owners." but the manifestival transform code does nothing to cluster-scoped owners.

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chinhuang007, njhill

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

@njhill
Copy link
Member

njhill commented Oct 6, 2022

/lgtm

@kserve-oss-bot kserve-oss-bot merged commit da45d91 into kserve:main Oct 6, 2022
@njhill
Copy link
Member

njhill commented Oct 6, 2022

FWIW I opened an issue in the manifestival repo to ask about this: manifestival/manifestival#90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants