Skip to content

feat: add new field RolloutStrategy control automatic rollout#202

Merged
hessjcg merged 4 commits intomainfrom
gh-187-strategy
Feb 8, 2023
Merged

feat: add new field RolloutStrategy control automatic rollout#202
hessjcg merged 4 commits intomainfrom
gh-187-strategy

Conversation

@hessjcg
Copy link
Copy Markdown
Collaborator

@hessjcg hessjcg commented Feb 8, 2023

Adds a new field RolloutStrategy to the AuthProxyWorkload that will allow users to control how
the operator will roll out changes when an AuthProxyWorkload is updated.

RolloutStrategy has two possible values:

  • Workload in which the operator will automatically follow the behavior of the Strategy set on the workload.
  • None in which the operator will not attempt to roll out changes

@hessjcg hessjcg requested a review from a team as a code owner February 8, 2023 22:10
description: 'Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
rolloutStrategy:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is generated from the source code comments: authproxyworkload_types.go

description: 'Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
type: object
type: object
rolloutStrategy:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is generated from the source code comments: authproxyworkload_types.go

@hessjcg hessjcg requested a review from enocom February 8, 2023 22:14
Copy link
Copy Markdown
Member

@enocom enocom 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 no e2e test here because the cluster is defaulting to "Workload" and so we can't test None?

func (r *AuthProxyWorkload) Default() {
authproxyworkloadlog.Info("default", "name", r.Name)
// TODO(user): fill in your defaulting logic.
if r.Spec.AuthProxyContainer != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if x != nil && y == "" {
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}

// The user has set "None" as the rollout strategy. Ignore it.
if resource.Spec.AuthProxyContainer != nil &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This check is pretty simple, but since we're repeating it and it's a pretty important check, should we pull it out into a re-used function?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Fixed.

t.Fatal(err)
}

// Fetch the deployment and make sure the annotations show the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This means the annotations are gone, but we haven't rolled it out, yeah?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This demonstrates the intended behavior. When RolloutStrategy== "None", then Reconcile() will not modify the Deployment's annotations.

@hessjcg
Copy link
Copy Markdown
Collaborator Author

hessjcg commented Feb 8, 2023

Regarding E2E tests: In my judgement, an E2E test case doesn't add a lot of additional quality assurance above the unit test, and it introduces another source of flakey test errors. I'm inclined to skip the e2e test.

For me, the unit test that shows that the Reconcile() will not modify a deployment when RolloutStrategy == "None" is enough.

@hessjcg hessjcg requested a review from enocom February 8, 2023 22:31
Labels: map[string]string{labelK: labelV},
},
Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}},
Copy link
Copy Markdown
Member

@enocom enocom Feb 8, 2023

Choose a reason for hiding this comment

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

Can we add some annotations here to just show that they're still there down below?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@hessjcg hessjcg requested a review from enocom February 8, 2023 22:40
@hessjcg hessjcg merged commit 090b88d into main Feb 8, 2023
@hessjcg hessjcg deleted the gh-187-strategy branch February 8, 2023 22:46
hessjcg pushed a commit that referenced this pull request Feb 21, 2023
Features
- Add new field RolloutStrategy control automatic rollout (#202) (090b88d)
- Add new terraform project for e2e test resources (#181) (0140592)
- Add script to run terraform with input validation. (#182) (857444a)
- Add support for Unix sockets. (#205) (8177a35), closes #47
- Add telemetry settings to configure health check port (#210) (3ede42d)
- Add the e2e test job for Cloud Build (#184) (dc2990c)
- Automatic changes to workloads when an AuthProxyWorload is deleted (#200) (e11caed)
- Automatically trigger pod rollout for appsv1 resources when AuthProxyWorkload changes. (#197) (3b0359b)
- Separate terraform for project setup and permissions (#179) (8f43657)
- Validate AuthProxyWorkload spec.selector field (#209) (98c460b)
- Validate AuthProxyWorkload updates to prevent changes to the workload selector. (#211) (4304283)

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Release PR Generate Bot action release-please[bot] <release-please[bot]@users.noreply.github.com>
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