search

kubeflow / manifests

A repository for Kustomize manifests
Apache License 2.0
772 stars 836 forks source link

Update kubeflow/kubeflow manifests from v1.9.0-rc.0 #2734

Closed kimwnasptd closed 3 weeks ago

kimwnasptd commented 1 month ago

This PR updates the manifests from kubeflow/kubeflow components in the manifests for 1.9.

The manifests are copied from https://github.com/kubeflow/kubeflow/releases/tag/v1.9.0-rc.0

/cc @thesuperzapper @juliusvonkohout @rimolive

google-oss-prow[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from kimwnasptd. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/kubeflow/manifests/blob/v1.9-branch/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
juliusvonkohout commented 1 month ago

@kimwnasptd looks good on a first glimpse. The failing test is the only major thing.

kimwnasptd commented 1 month ago

@juliusvonkohout the failure is unrelated to this PR though.

I see the following

Error: accumulating resources: accumulation err='accumulating resources from '../apps/centraldashboard/upstream/overlays/oauth2-proxy': evalsymlink failure on '/home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy' : lstat /home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy: no such file or directory': must build at directory: not a valid directory: evalsymlink failure on '/home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy' : lstat /home/runner/work/manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy: no such file or directory

Are the oauth2-proxy manifests as expected?

kimwnasptd commented 1 month ago

It looks like a manifest file was introduced in the manifests repo, but was not part of the source manifests https://github.com/kubeflow/manifests/blob/v1.9-branch/apps/centraldashboard/upstream/overlays/oauth2-proxy/ https://github.com/kubeflow/kubeflow/tree/v1.9-branch/components/centraldashboard/manifests/overlays

Is this the intended behavior?

juliusvonkohout commented 1 month ago

It looks like a manifest file was introduced in the manifests repo, but was not part of the source manifests https://github.com/kubeflow/manifests/blob/v1.9-branch/apps/centraldashboard/upstream/overlays/oauth2-proxy/ https://github.com/kubeflow/kubeflow/tree/v1.9-branch/components/centraldashboard/manifests/overlays

Is this the intended behavior?

It is used to patch the logout https://github.com/kubeflow/manifests/blob/v1.9-branch/common/oidc-client/oauth2-proxy/components/central-dashboard/patches/deployment.logout-url.yaml

we need to find a new place for that patch, because it should be maintained within kubeflow/manifests. You can also readd it for now and we tackle that in RC.2.

CC @kromanow94

rimolive commented 4 weeks ago

The error is in an overlay for oauth2 for central dashboard. I thought this was tested in kubeflow/kubeflow repo before, wasn't it?

EDIT: I got the whole thread now. What was the final decision about it? It looks like a simple change, so what's the ETA to get this done?

juliusvonkohout commented 4 weeks ago

The error is in an overlay for oauth2 for central dashboard. I thought this was tested in kubeflow/kubeflow repo before, wasn't it?

EDIT: I got the whole thread now. What was the final decision about it? It looks like a simple change, so what's the ETA to get this done?

I am in favour of just re-adding it (5 minutes of work) for now and fixing it in RC.2. RC.2 can be used for that and upgrading istio and the sometimes slow oauth2-proxy installation. Let's resynchronize on Monday for that.

juliusvonkohout commented 3 weeks ago

@kimwnasptd Lets close this. It is syncing into the wrong branch. We need to sync to master first and then from master into 1.9. Otherwise we end up in synchronization hell ;-)

@rimolive Can you run the script to create a new PR against the master branch and undelete /manifests/manifests/apps/centraldashboard/upstream/overlays/oauth2-proxy.

Then we need to synchronize the 1.9 branch to master and you can tag RC.1 this week with https://github.com/kubeflow/manifests/pull/2739

rimolive commented 3 weeks ago

As per Julius suggestion, will close this PR and create a new one with the missing manifest.

/close

google-oss-prow[bot] commented 3 weeks ago

@rimolive: Closed this PR.

In response to [this](https://github.com/kubeflow/manifests/pull/2734#issuecomment-2145833601): >As per Julius suggestion, will close this PR and create a new one with the missing manifest. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
thesuperzapper commented 3 weeks ago

@rimolive @juliusvonkohout aside the PR target issue (the PR should obviously be into master).

It's probably better if we just move anything that was manually added to the "upstream" folder (this ouath2-proxy stuff) into its own folder, so we can sync easily in the future?

rimolive commented 3 weeks ago

@thesuperzapper I agree with that. @juliusvonkohout let's work on this in future releases, or check if it's possible to make that change for 1.9.

juliusvonkohout commented 3 weeks ago

Yes i hope so for 1.9 RC.2