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

fix: Pluralize topologySpreadConstraint to match docs #1089

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

cbui
Copy link
Contributor

@cbui cbui commented Feb 3, 2022

Original PR:
https://github.com/actions-runner-controller/actions-runner-controller/pull/814/files#diff-25283fab3c6d5fa726652c8741a122c1ba14d8486fe092774617a385e4bc1a92R145

Fixes: #984

This is a breaking change for users that have figured out that the field should have been singular.

@@ -145,7 +145,7 @@ type RunnerPodSpec struct {
HostAliases []corev1.HostAlias `json:"hostAliases,omitempty"`

// +optional
TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraint,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey! Thanks for the correction.
Could you instead add a new field so that we can safely remove it in a later version (like v1alpha2)?

@BeyondEvil
Copy link

Any updates here? We would like to get this sorted and are happy to help! @cbui @mumoshu

@mumoshu
Copy link
Collaborator

mumoshu commented Mar 23, 2022

@BeyondEvil Hey! It would be great if you could submit another pull request as a successor to this. In that PR code after the change should look like this:

// +optional
TopologySpreadConstraint []corev1.TopologySpreadConstraint `json:"topologySpreadConstraint,omitempty"`

// +optional
TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`

And in our controller we should read topologySpreadConstraint as topologySpreadConstraints(notice the trailing s), by assigning what's in the legacy topologySpreadConstraint to the new topologySpreadConstraints.

@mumoshu mumoshu force-pushed the fix-topology-spread-constraints branch from 65ca082 to 6ecb7f7 Compare April 20, 2022 01:34
@mumoshu
Copy link
Collaborator

mumoshu commented Apr 20, 2022

I discussed with @toast-gear and decided to just make it a breaking change, assuming not everyone uses this feature and ARC is still pre 1.0.
We'll include this change since ARC v0.23.0. Before upgrading, any users of this feature should stop ARC, upgrade CRDs, update RunnerDeployments, and finally upgrade and restart ARC. This way, there will be no downtime for already running workflow jobs.

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @cbui ☺️

@mumoshu mumoshu merged commit cb4e1fa into actions:master Apr 20, 2022
@BeyondEvil
Copy link

@BeyondEvil Hey! It would be great if you could submit another pull request as a successor to this. In that PR code after the change should look like this:

// +optional
TopologySpreadConstraint []corev1.TopologySpreadConstraint `json:"topologySpreadConstraint,omitempty"`

// +optional
TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"`

And in our controller we should read topologySpreadConstraint as topologySpreadConstraints(notice the trailing s), by assigning what's in the legacy topologySpreadConstraint to the new topologySpreadConstraints.

Hey @mumoshu

Do you still need these changes? If so, I might be able to get them done today.

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.

Type in topologySpreadConstraint configuration of runner deployment
3 participants