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

Support deploying provision probe or mount probe only #132

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

ushitora-anqou
Copy link
Contributor

Currently, when a user deploys a PieProbe object, both of provision and
mount probes are deployed. However, under some circumstances, it is
enough to deploy either provision probes or mount probes only.

This commit supports deploying either provision probes or mount probes
only, by adding disableProvisionProbes and disableMountProbes fields to
PieProbe spec.

Signed-off-by: Ryotaro Banno ryotaro.banno@gmail.com

@ushitora-anqou ushitora-anqou changed the title Support deploying provision probe only Support deploying provision probe or mount probe only Aug 6, 2024
@ushitora-anqou ushitora-anqou force-pushed the support-deploying-provision-probe-only branch 3 times, most recently from 0541fa9 to 6828adf Compare August 13, 2024 02:38
@ushitora-anqou ushitora-anqou marked this pull request as ready for review August 13, 2024 04:21
@ushitora-anqou ushitora-anqou requested a review from a team as a code owner August 13, 2024 04:21
Copy link
Contributor

@daichimukai daichimukai left a comment

Choose a reason for hiding this comment

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

I'm confused with namings of added fields and something like that. Are they expected to be singular form, or plural form?

  • As struct fields, both are plural: DisableProvisionProbes, DisableMountProbes.
  • As yaml fields, both are singular: disableProvisionProbe, disableMountProbe.
  • Reconcilers have mixed form: reconcileProvisionProbe, reconcileMountProbes.

At least it seems to me that we should use singular form for the "provision probe" as it creates exactly one CronJob. On the other hand I think both singular form and plural form are reasonable for the "mount probe". I'm happy if only one of them is consistently used, anyway.

Currently, when a user deploys a PieProbe object, both of the provision
probe and the mount probes are deployed. However, under some
circumstances, it is enough to deploy either the provision probe or the
mount probes only.

This commit supports deploying either the provision probe or the mount
probes only, by adding disableProvisionProbe and disableMountProbes
fields to PieProbe spec.

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
@ushitora-anqou ushitora-anqou force-pushed the support-deploying-provision-probe-only branch from 6828adf to 2306138 Compare August 13, 2024 08:31
@ushitora-anqou
Copy link
Contributor Author

ushitora-anqou commented Aug 13, 2024

Thanks for your feedback. I fixed my code to use the names disableProvisionProbe (without "s") and disableMountProbes (with "s") only.

@ushitora-anqou ushitora-anqou merged commit 94a9a76 into main Aug 14, 2024
7 checks passed
@ushitora-anqou ushitora-anqou deleted the support-deploying-provision-probe-only branch August 14, 2024 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants