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.

Sign up for GitHub

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

Jump to bottom

Extract the registry deployment into component. Add missing unit tests #36

Merged
merged 6 commits into from
Sep 8, 2023
Merged

Extract the registry deployment into component. Add missing unit tests #36

merged 6 commits into from
Sep 8, 2023

Conversation

ialidzhikov
Copy link
Member

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:
This PR:

  • Adds unit tests for ./pkg/admission/validator.
  • Extracts the registry deployment into component following the well-known component model from gardener/gardener. Adds unit tests for the new component.
  • Moves the existing controller from ./pkg/controller to dedicated pkg ./pkg/controller/extension. The controller name is changed from registry-cache to extension-controller.
  • Introduces a new pkg ./pkg/constants. It exports constants that are used from 2 or more packages.

Which issue(s) this PR fixes:
Part of #3

Special notes for your reviewer:
N/A

Release note:

NONE

@gardener-prow gardener-prow bot added the kind/enhancement Enhancement, improvement, extension label Aug 25, 2023
@gardener-prow gardener-prow bot added cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 25, 2023
@ialidzhikov ialidzhikov mentioned this pull request Aug 25, 2023
Closed
96 tasks
@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023
@gardener-prow gardener-prow bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 30, 2023
@ialidzhikov
Require at least one cache to be specified
2f55d1f
@ialidzhikov
Add unit tests for the shoot validator
bc3837b
@ialidzhikov
Fix addDefaultingFuncs for the config API
6293c54
@ialidzhikov
Remove test suite for package that does not define any tests
8e70eb0
@ialidzhikov
Extract the registry deployment into component. Add unit tests for th…
d47ee7c 
…e new component

A new component is introduced in `./pkg/component/registrycaches`. The component deploys registry caches. It implements the well-known `component.DeployWaiter` interface.

This commit also moves the existing controller from `./pkg/controller` to dedicated pkg `./pkg/controller/extension`. The controller name is changed from `registry-cache` to `extension-controller`.
This commit also introduces a new pkg `./pkg/constants`. It exports constants that are used from 2 or more packages.
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2023
@ialidzhikov
Copy link
Member Author

/test pull-gardener-extension-registry-cache-e2e-kind

@ialidzhikov
Copy link
Member Author

Maybe it would make sense to have envtest for the webhook instead of unit tests. Let me check on this.

/hold

@gardener-prow gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2023
@ialidzhikov
Copy link
Member Author

I had a quick look. It seems that envtest.GardenerTestEnvironment does not have a native support for webhook installation such that is present in envtest.Environment (see https://github.com/gardener/gardener/blob/809fd994e01455eb9f67af35f195bab76ebade2f/test/integration/resourcemanager/podschedulername/podschedulername_suite_test.go#L69-L74).
So, I guess unit test is good enough for now.

/unhold

@gardener-prow gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2023
plkokanov
plkokanov suggested changes Sep 5, 2023
View reviewed changes
Copy link

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

Nice PR! I just saw some minor nits.

pkg/component/registrycaches/registrycaches.go
}

func computeResourcesDataForRegistryCache(cache *v1alpha1.RegistryCache, image string) ([]client.Object, error) {
if cache.Size == nil {
Copy link

Choose a reason for hiding this comment

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

This is checked in the validation, must we also check for it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to follow defensive programming paradigm to fail with a more meaningful error instead of a panic (would cause a crash of the whole process) if the defaulting or validation (in registry-cache-admission) don't work for some reason.

Copy link

Choose a reason for hiding this comment

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

Alright, I just remember from some reviews that we did in g/g that we avoid such checks when there is validation/defaulting, but I don't really mind tbh.

pkg/component/registrycaches/registrycaches.go
if cache.Size == nil {
return nil, fmt.Errorf("registry cache size is required")
}
if cache.GarbageCollectionEnabled == nil {
Copy link

Choose a reason for hiding this comment

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

This is defaulted to not be nil. Is this check necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as #36 (comment).

pkg/component/registrycaches/registrycaches.go Outdated Show resolved Hide resolved
pkg/component/registrycaches/registrycaches_test.go Outdated Show resolved Hide resolved
vendor/github.com/gardener/gardener/pkg/mock/controller-runtime/client/doc.go Show resolved Hide resolved
plkokanov
plkokanov approved these changes Sep 7, 2023
View reviewed changes
Copy link

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2023
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 7, 2023

LGTM label has been added.

Git tree hash: 6f57588ff020eb88dc2cc2c47c74178a7d36858d

@ialidzhikov
Copy link
Member Author

/approve

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 8, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, plkokanov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2023
@gardener-prow gardener-prow bot merged commit dc34900 into gardener:main Sep 8, 2023
@ialidzhikov ialidzhikov deleted the enh/unit-tests branch September 8, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plkokanov plkokanov plkokanov approved these changes

@dimitar-kostadinov dimitar-kostadinov Awaiting requested review from dimitar-kostadinov

@Kostov6 Kostov6 Awaiting requested review from Kostov6

Assignees

@plkokanov plkokanov

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ialidzhikov @plkokanov