-
Notifications
You must be signed in to change notification settings - Fork 100
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
Target StatefulSet without copy_target. #2536
base: main
Are you sure you want to change the base?
Target StatefulSet without copy_target. #2536
Conversation
meowjesty
commented
Jun 18, 2024
- Issue: 544
- Small refactor on the cli getting a list of (kube) resources;
- Adds statefulset compliance to verify-config;
Create an issue to check for new targets for after we finish the backward compatibility fixes with operator (jobs,cronjobs,statefulsets should be tested only when operator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits, needs rebasing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing
mirrord/cli/src/verify_config.rs
Outdated
"{verified_target} target requires the mirrord-operator, but the config \ | ||
was set to `operator: false`! Consider setting `operator: true` for \ | ||
this target type." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"{verified_target} target requires the mirrord-operator, but the config \ | |
was set to `operator: false`! Consider setting `operator: true` for \ | |
this target type." | |
format!("{} targets require the mirrord-operator, but operator usage was explicitly disabled. Consider enabling mirrord-operator in your mirrord config.", verified_target.target_type()) |
Requires implementing VerifiedTarget::target_type(&self) -> TargetType
and Display for TargetType
but would be nicer, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. I ended up impl'ing Into
, wdyt?
fn requires_operator(&self) -> bool { | ||
matches!(self, Self::CronJob(_) | Self::Job(_) | Self::StatefulSet(_)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this check be in LayerConfig::verify_config
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean in here pub fn verify(&self, context: &mut ConfigContext) -> Result<(), ConfigError>
, instead of in the verify_config
?