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

setting statuses, and triggering reconciliation #17

Merged
merged 16 commits into from
Feb 1, 2024

Conversation

kristofgyuracz
Copy link
Collaborator

  • scratch implementation
  • api changes
  • set tenant-> collector ref and notify controller on change
  • triggering for relevant changes, disowning, and checking ownership before adopting tenants and subscriptions

pepov and others added 4 commits January 25, 2024 13:12
Signed-off-by: Peter Wilcsinszky <peter.wilcsinszky@axoflow.com>
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
…fore adopting tenants and subscriptions

Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz self-assigned this Jan 29, 2024
Signed-off-by: Kristóf Gyurácz <4431202+kristofgyuracz@users.noreply.github.com>
Copy link
Member

@pepov pepov left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, however we need to make sure we handle errors consistently.

Generally there is nothing wrong with returning an error and waiting for the next reconcile to fix it, but in this case we build a system where a misconfigured tenant/subscription should have the least possible effect on the other actors.
In this case if one subscription cannot properly be updated, then we get stuck and no configuration changes can be made anymore on the collector in general.

Update: we just want to handle subscription update errors in this special way to make sure a user updating their subscription won't be able to intentionally or accidentally block the system

internal/controller/telemetry/collector_controller.go Outdated Show resolved Hide resolved
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch 2 times, most recently from 42f5993 to fccde59 Compare January 30, 2024 14:14
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch from fccde59 to 79aefac Compare January 30, 2024 14:26
Copy link
Collaborator

@OverOrion OverOrion left a comment

Choose a reason for hiding this comment

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

Overall LGTM, two minor notes only.

Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch from 331bdbd to 122806f Compare January 30, 2024 14:58
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
pepov and others added 2 commits January 31, 2024 14:02
Signed-off-by: Peter Wilcsinszky <peter.wilcsinszky@axoflow.com>
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch 4 times, most recently from d4f96da to 3036752 Compare January 31, 2024 17:16
@kristofgyuracz kristofgyuracz requested a review from pepov January 31, 2024 17:17
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch from 3036752 to 8e67060 Compare January 31, 2024 17:22
kristofgyuracz and others added 3 commits January 31, 2024 18:27
Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
Signed-off-by: Peter Wilcsinszky <peter.wilcsinszky@axoflow.com>
Signed-off-by: Peter Wilcsinszky <peter.wilcsinszky@axoflow.com>
Signed-off-by: Peter Wilcsinszky <peter.wilcsinszky@axoflow.com>
Copy link
Member

@pepov pepov left a comment

Choose a reason for hiding this comment

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

looks great, a few nits I've added: put arm64 image build back and equality checks before collector/tenant updates

This reverts commit 71834f8.

Signed-off-by: Kristof Gyuracz <kristof.gyuracz@gmail.com>
@kristofgyuracz kristofgyuracz force-pushed the status-and-notification-notes branch from b0fcce1 to 4bd14ce Compare February 1, 2024 13:21
@kristofgyuracz kristofgyuracz merged commit 07b841c into main Feb 1, 2024
13 checks passed
@kristofgyuracz kristofgyuracz deleted the status-and-notification-notes branch February 1, 2024 13:45
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.

3 participants