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 scheduling logic #25

Merged
merged 31 commits into from
Mar 2, 2021
Merged

Add scheduling logic #25

merged 31 commits into from
Mar 2, 2021

Conversation

maruina
Copy link
Contributor

@maruina maruina commented Feb 23, 2021

Fix #20

Notes:

  • Fix local development instructions with kubebuilder
  • Fix indentation in README
  • Change the function name to Owns, because then when we read pr.Owns(app.GetOwnersReference()) is clear that the Progressive Rollout owns the Application
  • A scheduler that computes which applications we need to updated based on the stage parameters
  • Scheduler tests
  • New utils functions with their tests
  • Updated the should reconcile integration test to work with the scheduler

@maruina maruina marked this pull request as ready for review February 26, 2021 13:09
@maruina maruina force-pushed the scheduling branch 2 times, most recently from 8932359 to 729413a Compare February 26, 2021 13:42
Copy link
Contributor

@DiogoMCampos DiogoMCampos left a comment

Choose a reason for hiding this comment

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

Added some comments and thoughts. In general it looks fine, there's a few clarifications (around App vs App Set, test cases, etc.) that might be needed and minor suggestions on comments and naming.

api/v1alpha1/progressiverollout_types_test.go Show resolved Hide resolved
internal/utils/utils.go Show resolved Hide resolved
controllers/progressiverollout_controller.go Show resolved Hide resolved
controllers/progressiverollout_controller.go Outdated Show resolved Hide resolved
internal/scheduler/scheduler.go Show resolved Hide resolved
internal/scheduler/scheduler_test.go Outdated Show resolved Hide resolved
internal/scheduler/scheduler_test.go Outdated Show resolved Hide resolved
internal/scheduler/scheduler_test.go Show resolved Hide resolved
controllers/progressiverollout_controller.go Show resolved Hide resolved
internal/utils/utils_test.go Show resolved Hide resolved
controllers/progressiverollout_controller.go Outdated Show resolved Hide resolved
internal/scheduler/scheduler.go Outdated Show resolved Hide resolved
internal/utils/utils.go Outdated Show resolved Hide resolved
internal/utils/utils.go Outdated Show resolved Hide resolved
@maruina maruina mentioned this pull request Mar 1, 2021
@sledigabel
Copy link
Collaborator

One more comments, on []Applications and stage types in functions, we seem to oscillate between pointer and value semantics.
I'd suggest we use pointer semantics everywhere for more clarity and avoid potential issues that will get very difficult to debug as the code base keeps expanding.

https://github.com/Skyscanner/argocd-progressive-rollout/pull/25/files#diff-b1a5cf22f2be07477047c7c268c9a2077f75c3fdadc285eb862dab8bc3aaad31R293
https://github.com/Skyscanner/argocd-progressive-rollout/pull/25/files#diff-c3753ff27e79b7da6571efc4fffc45e729b031a39337eec9b22a04425bbd69f0R12

I am not sure if there are standards around that specifically for operators, but generally speaking when dealing with structures that have a degree of identity to it (typically, an Application has an identity, it's deploying something to somewhere, and nothing else does that), Go tends to push you towards using pointer semantics.

@maruina
Copy link
Contributor Author

maruina commented Mar 1, 2021

I was using pointers where I was doing in-place update of a data structure, this is why we have func (r *ProgressiveRolloutReconciler) removeAnnotationFromApps(apps *[]argov1alpha1.Application, annotation string) error {...}

Regarding pointers vs value, I was using the majority of value because I thought it was easier to understand and I don't see any downside for our workload.

@sledigabel
Copy link
Collaborator

As discussed, would be good to have a wider conversation about it, as it'll cause some potentially heavy refactor.
but rather do it early than later.
Filed #33 for a follow up conversation

Copy link
Collaborator

@sledigabel sledigabel left a comment

Choose a reason for hiding this comment

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

LGTM, adds a scheduling capability to the reconcile

@maruina maruina merged commit 9d9523d into main Mar 2, 2021
@maruina maruina deleted the scheduling branch March 2, 2021 10:22
maruina added a commit that referenced this pull request Mar 3, 2021
Co-authored-by: Diogo Campos <d.diogocampos@gmail.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.

Add scheduling logic
4 participants