-
Notifications
You must be signed in to change notification settings - Fork 187
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
Rewrite BucketReconciler
to new standards
#412
Conversation
ca456d3
to
cd886e3
Compare
BucketReconcilers
to new standardsBucketReconciler
to new standards
cd886e3
to
4b82782
Compare
cb7b730
to
3123992
Compare
}, timeout).Should(BeTrue()) | ||
} | ||
|
||
func TestBucketReconciler_reconcileStorage(t *testing.T) { |
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.
reconcileStorage()
implementations for bucket and gitrepository reconcilers seem to be exactly the same. Should we have a common function with this one test? Currently there's no test for reconcileStorage
in gitrepository reconciler.
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.
I had noticed this too, but not taken the time yet to think about how I wanted to factor it out. The same goes for garbageCollect
by the way.
8bc86ba
to
4338305
Compare
4338305
to
0d2ae53
Compare
75f555c
to
20bf5da
Compare
0016698
to
8e2fdec
Compare
This commit rewrites the `BucketReconciler` to new standards, while implementing the newly introduced Condition types, and trying to adhere better to Kubernetes API conventions. More specifically it introduces: - Implementation of more explicit Condition types to highlight abnormalities. - Extensive usage of the `conditions` subpackage from `runtime`. - Better and more conflict-resilient (status)patching of reconciled objects using the `patch` subpackage from runtime. - Proper implementation of kstatus' `Reconciling` and `Stalled` conditions. - Refactor of reconciler logic, including more efficient detection of changes to bucket objects by making use of the etag data available, and downloading of object files in parallel with a limited number of workers (4). - Integration tests that solely rely on `testenv` and do not use Ginkgo. There are a couple of TODOs marked in-code, these are suggestions for the future and should be non-blocking. In addition to the TODOs, more complex and/or edge-case test scenarios may be added as well. Signed-off-by: Hidde Beydals <hello@hidde.co>
This commit consolidates the `DownloadFailed` and `CheckoutFailed` Condition types into a new more generic `FetchFailed` type to simplify the API and observations by consumers. Signed-off-by: Hidde Beydals <hello@hidde.co>
Add `BucketReconciler.reconcileArtifact` tests based on `GitRepositoryReconciler.reconcileArtifact` test cases. Signed-off-by: Sunny <darkowlzz@protonmail.com>
8e2fdec
to
3f2fcf3
Compare
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.
LGTM
Thanks @hiddeco ☕
This wraps the errors which are returned instead of logging them, as the returned error is logged at the end of the reconcile run. Signed-off-by: Hidde Beydals <hello@hidde.co>
3f2fcf3
to
29f207d
Compare
Supersedes #361
This PR rewrites the
BucketReconciler
to new standards, whileimplementing the newly introduced Condition types, and trying to
adhere better to Kubernetes API conventions.
More specifically it introduces:
abnormalities.
conditions
subpackage fromruntime
.objects using the
patch
subpackage from runtime.Reconciling
andStalled
conditions.
changes to bucket objects by making use of the etag data available,
and downloading of object files in parallel with a limited number of
workers (4).
testenv
and do notuse Ginkgo.
There are a couple of TODOs marked in-code, these are suggestions for
the future and should be non-blocking.
In addition to the TODOs, more complex and/or edge-case test scenarios
may be added as well.