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

feat(kubernetes_platform): Add empty dir mount #10892

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

gregsheremeta
Copy link
Contributor

Description of your changes:
Update kubernetes_platform go package to include the ability to add an EmptyDir mount.

Part 1 of 2. Will be rebased when the backend part is merged. Posting as a WIP now to get feedback on the fields in the proto message.

Following the example set in #10410.

Checklist:

Copy link

Hi @gregsheremeta. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@gregsheremeta
Copy link
Contributor Author

@chensun do you know whom I can nag about reviewing kfp-kubernetes PRs? I ask because it seems like Connor left the project.

@hbelmiro
Copy link
Contributor

/ok-to-test

Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -38,6 +38,7 @@ message KubernetesExecutorConfig {
repeated NodeAffinityTerm node_affinity = 14;
repeated PodAffinityTerm pod_affinity = 15;
EnabledSharedMemory enabled_shared_memory = 16;
repeated EmptyDirMount empty_dir_mount = 17;
Copy link
Member

Choose a reason for hiding this comment

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

nit: empty_dir_mounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting. I don't see plurals anywhere else -- for example, line 26:

repeated PvcMount pvc_mount = 3;

that said, this is my very first time doing anything with protobuf, so if repeated fields should be pluralized, I will pluralize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref: https://cloud.google.com/apis/design/naming_convention#repeated_field_names

Repeated fields in APIs must use proper plural forms. This matches the convention of existing Google APIs, and the common expectation of external developers.

Part of kubeflow#10656

Signed-off-by: Greg Sheremeta <gshereme@redhat.com>
Signed-off-by: Greg Sheremeta <gshereme@redhat.com>
Copy link

@gregsheremeta: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
kfp-kubernetes-execution-tests 9ba9bdd link false /test kfp-kubernetes-execution-tests

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-sigs/prow repository. I understand the commands that are listed here.

@gregsheremeta gregsheremeta changed the title WIP: feat(kubernetes_platform): Add empty dir mount feat(kubernetes_platform): Add empty dir mount Jun 17, 2024
@gregsheremeta
Copy link
Contributor Author

s/WIP//

I think it's good for final review

@gregsheremeta
Copy link
Contributor Author

@chensun can you re-lgtm this? I made the empty_dir_mount -> empty_dir_mounts change you asked for, and the lgtm was removed

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@chensun chensun left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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

@google-oss-prow google-oss-prow bot merged commit 10aaf43 into kubeflow:master Jul 24, 2024
6 of 8 checks passed
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