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

Add CronJob and StatefulSet targetting when copy_target is enabled. #2493

Merged
merged 12 commits into from
Jun 14, 2024

Conversation

meowjesty
Copy link
Member

@meowjesty meowjesty commented Jun 4, 2024

  1. Adds CronJob as a possible target (requires copy_target feature to be enabled);
  2. Adds StatefulSet as a possible target (requires copy_target feature to be enabled);

@meowjesty meowjesty requested a review from Razz4780 June 11, 2024 20:52
Copy link
Contributor

@Razz4780 Razz4780 left a comment

Choose a reason for hiding this comment

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

There's also one thing I missed when reviewing previous JobTarget PR - we need a feature flag in the operator status to check whether this target type is supported

mirrord/config/src/target.rs Outdated Show resolved Hide resolved
mirrord/config/src/target/cron_job.rs Outdated Show resolved Hide resolved
@Razz4780
Copy link
Contributor

Issue not found ;/

@aviramha
Copy link
Member

There's also one thing I missed when reviewing previous JobTarget PR - we need a feature flag in the operator status to check whether this target type is supported

We could trust the mirrord ls in terms of UX, no?

@Razz4780
Copy link
Contributor

There's also one thing I missed when reviewing previous JobTarget PR - we need a feature flag in the operator status to check whether this target type is supported

We could trust the mirrord ls in terms of UX, no?

Yes, job targets should not be listed unless operator supports them

@aviramha
Copy link
Member

There's also one thing I missed when reviewing previous JobTarget PR - we need a feature flag in the operator status to check whether this target type is supported

We could trust the mirrord ls in terms of UX, no?

Yes, job targets should not be listed unless operator supports them

I'm saying we don't need a feature flag because it either returns it as an option or it doesn't.

@aviramha
Copy link
Member

If we want a feature flag for nicer error, I'd make a "target not supported" error with appropriate message and re-use it, as we're adding more and more targets, and it's a bit weird to have a feature flag for each

@Razz4780
Copy link
Contributor

If we want a feature flag for nicer error, I'd make a "target not supported" error with appropriate message and re-use it, as we're adding more and more targets, and it's a bit weird to have a feature flag for each

Okok, makes sense

@meowjesty meowjesty requested a review from Razz4780 June 12, 2024 18:10
mirrord/config/src/lib.rs Outdated Show resolved Hide resolved
mirrord/config/src/target.rs Show resolved Hide resolved
mirrord/config/src/target.rs Outdated Show resolved Hide resolved
mirrord/kube/src/api/runtime/stateful_set.rs Outdated Show resolved Hide resolved
@meowjesty meowjesty changed the title Add CronJob targetting when copy_target is enabled. Add CronJob and StatefulSet targetting when copy_target is enabled. Jun 14, 2024
@meowjesty meowjesty enabled auto-merge June 14, 2024 17:58
@meowjesty meowjesty added this pull request to the merge queue Jun 14, 2024
Merged via the queue into metalbear-co:main with commit 4fdca3f Jun 14, 2024
16 checks passed
@meowjesty meowjesty deleted the issue/404/target_cronjob branch June 14, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants