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

update kfservice status URL #289

Merged
merged 1 commit into from
Aug 19, 2019
Merged

update kfservice status URL #289

merged 1 commit into from
Aug 19, 2019

Conversation

jinchihe
Copy link
Contributor

@jinchihe jinchihe commented Aug 15, 2019

What this PR does / why we need it:

Background: After introducing apis.URL to status in the #251, we have to create this hack since that struct is deep in go standard net library and we can not add openapi annotation, @yuzisun suggests creating our simplified URL struct. After investigation, I think that's better to back to string for URL since if we ceate our URL struct, that's hard to assign with net.url (cannot sure no issue in the future). And there is a String function in the knative.dev/pkg/apis.URL, we can use that and transfer to string for kfservice. That's good enough for kfservice.

See more details in the #272


This change is Reviewable

@jinchihe
Copy link
Contributor Author

/cc @yuzisun
/cc @johnugeorge

Could you please help to take a look? Thanks.

@yuzisun
Copy link
Member

yuzisun commented Aug 15, 2019

Sounds good to me, want to hear @johnugeorge’s thoughts.

@johnugeorge
Copy link
Contributor

Looks good to me.
/cc @ellis-bigelow
@ellis-bigelow had a comment in my initial PR to keep the same status type as the knative.

@k8s-ci-robot k8s-ci-robot requested a review from ellistarn August 15, 2019 13:40
@hougangliu
Copy link
Contributor

/lgtm

@yuzisun
Copy link
Member

yuzisun commented Aug 19, 2019

Discussed with Ellis we can always extend if needed.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuzisun

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 merged commit bbd3da4 into kserve:master Aug 19, 2019
@jinchihe jinchihe deleted the update_kfserivce_url branch August 19, 2019 00:56
ellistarn pushed a commit to ellistarn/kfserving that referenced this pull request Jul 28, 2020
rafvasq pushed a commit to rafvasq/kserve that referenced this pull request Jul 21, 2023
#### Motivation

The file containing the default configuration for the ModelMesh Controller is called `config-defaults.yaml`, which resides in a directory also called `config-defaults.yaml` as pointed out by @adrwong-lora in issue kserve#205
```
/etc/model-serving/config-defaults.yaml/config-defaults.yaml
```

#### Modifications

Change `mountPath` from `/etc/model-serving/config-defaults.yaml` to `/etc/model-serving/config/default`

#### Result

The file containing the default configuration file after this fix:

```
/etc/model-serving/config/default/config-defaults.yaml
```

Which more closely reflects the path of the [`config-defaults.yaml`](https://github.com/kserve/modelmesh-serving/blob/main/config/default/config-defaults.yaml) file in the `modelmesh-serving` repository:
```
config/default/config-defaults.yaml
```

Resolves kserve#205 

/cc @njhill 
/fyi @adrwong-lora

Signed-off-by: Christian Kadner <ckadner@us.ibm.com>
ruivieira pushed a commit to ruivieira/kserve that referenced this pull request Oct 1, 2024
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.

5 participants