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 PVCViewer manifests #7178

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Conversation

TobiasGoerke
Copy link
Contributor

@TobiasGoerke TobiasGoerke commented Jun 21, 2023

  • roleRef.name now matches role.metadata.name.
    Without this change, kustomize build doesn't correctly bind the resulting clusterrolebinding.
  • Remove creationTimestamp: null as noted by @axel7083
  • Use commonLabels over labels to comply with old kustomize version. Also, I had to introduce the deprecated vars feature as replacements aren't supported with the current kustomize version used by the build pipeline

See #6876 for initial PR.

@TobiasGoerke
Copy link
Contributor Author

/approve

@TobiasGoerke TobiasGoerke changed the title Update role_binding.yaml Update PVCViewer role_binding.yaml Jun 21, 2023
@axel7083
Copy link
Contributor

axel7083 commented Jul 20, 2023

While this PR is open and focused on fixing changed in yaml files, maybe take a look at the following:

controller-gen.kubebuilder.io/version: v0.10.0
creationTimestamp: null
name: pvcviewers.kubeflow.org

metadata:
creationTimestamp: null
name: role

Why creationTimestamp is null ? It prevents the resource to be deployed on my side.

PS: I can open an issue, if you do not want to fix in this PR
@TobiasGoerke

@TobiasGoerke
Copy link
Contributor Author

While this PR is open and focused on fixing changed in yaml files, maybe take a look at the following:

controller-gen.kubebuilder.io/version: v0.10.0
creationTimestamp: null
name: pvcviewers.kubeflow.org

metadata:
creationTimestamp: null
name: role

Why creationTimestamp is null ? It prevents the resource to be deployed on my side.

PS: I can open an issue, if you do not want to fix in this PR @TobiasGoerke

Good catch. Fixed, thanks!
kubebuilder adds these fields automatically, which is a well-known issue.

@google-oss-prow google-oss-prow bot added size/S and removed size/XS labels Jul 22, 2023
@TobiasGoerke TobiasGoerke changed the title Update PVCViewer role_binding.yaml Update PVCViewer manifests Jul 22, 2023
@google-oss-prow google-oss-prow bot added size/L and removed size/S labels Jul 22, 2023
@TobiasGoerke
Copy link
Contributor Author

/assign kimwnasptd

@TobiasGoerke
Copy link
Contributor Author

/lgtm
/approve

@google-oss-prow
Copy link

@TobiasGoerke: you cannot LGTM your own PR.

In response to this:

/lgtm
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TobiasGoerke

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

@kimwnasptd
Copy link
Member

@TobiasGoerke could you rebase on top of latest master now that #7219 is merged?

TobiasGoerke and others added 4 commits July 31, 2023 19:16
roleRef.name now matches role.metadata.name
to comply with old kustomize version being used in integration tests
@TobiasGoerke
Copy link
Contributor Author

@TobiasGoerke could you rebase on top of latest master now that #7219 is merged?

Done. Looking good now, both unit and integration tests run successfully

@kimwnasptd
Copy link
Member

Nice!

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jul 31, 2023
@google-oss-prow google-oss-prow bot merged commit 0d9e1ce into kubeflow:master Jul 31, 2023
4 checks passed
@TobiasGoerke TobiasGoerke deleted the patch-1 branch July 31, 2023 18:09
@TobiasGoerke
Copy link
Contributor Author

TobiasGoerke commented Jul 31, 2023

@kimwnasptd the docker build pipeline failed: https://github.com/kubeflow/kubeflow/actions/runs/5718096836. Same goes for the notebook controller which I copied the pipeline off with the same error: https://github.com/kubeflow/kubeflow/actions/runs/5717331751. Looks like there might be a superfluous comma in the platform arg?

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.

None yet

3 participants