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: add autoscaler service integration support #836

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Conversation

rriski
Copy link
Contributor

@rriski rriski commented Oct 29, 2024

  • Adds autoscaler service integration and service integration endpoint support

@rriski rriski requested a review from a team as a code owner October 29, 2024 14:33
@rriski rriski force-pushed the rriski-autoscaler branch from 9f121a8 to de975aa Compare October 30, 2024 10:32
@rriski
Copy link
Contributor Author

rriski commented Oct 30, 2024

tests

@byashimov
Copy link
Contributor

Hey. Looks good to me, few things I would do:

  1. put some validation into service update part, here generic_service_handler.go:
diskSpace := v1alpha1.ConvertDiskSpace(o.getDiskSpace())
if diskSpace > 0 {
	for _, v := range oldService.ServiceIntegrations {
		if v.IntegrationType == service.IntegrationTypeAutoscaler {
			return fmt.Errorf("cannot set disk space for service with autoscaler integration enabled")
		}
	}
}
  1. Add a warning here on the disk space field description common.go, something like "should not be set while the autoscaler service integration is running"

Additionally we could have webhook validation, but the setup manager does not passthrough the go client. So let's not do that.

Copy link
Contributor

@byashimov byashimov left a comment

Choose a reason for hiding this comment

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

Please add disk_space vs autoscaler validation 🙏

@rriski rriski requested a review from byashimov November 6, 2024 12:24
@rriski rriski dismissed byashimov’s stale review November 6, 2024 12:24

Changes addressed

@byashimov byashimov merged commit 208d95c into main Nov 6, 2024
7 checks passed
@byashimov byashimov deleted the rriski-autoscaler branch November 6, 2024 14:35
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.

2 participants