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

feat: adding annotation to define reconciliation frequency #297

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: adding annotation to define reconciliation frequency #297

wants to merge 1 commit into from

Conversation

ADorigi
Copy link

@ADorigi ADorigi commented Sep 17, 2024 •
edited
Loading

Issue

partially resolves validator-labs/validator#358

Description

  • Added the annotation validation.validator.labs/reconciliation-frequency.
  • The annotation value is an integer representing the time in seconds.
  • I added a constant to define the annotation key.
  • The controller checks if the annotation has been defined. If it finds the annotation, it uses that. If it doesn't find the annotation, it takes 120 seconds as default.
  • Added a sample OciValidator manifest as an example.

@ADorigi
feat: adding frequency annotation
fd30865 
added an example for using the reconciliation frequency

added constant to define annotation key

controller checks if annotation has been defined,
if annotation is present, the value is considered to be in seconds
if annotation is not present, 120 seconds is taken as default value
@ADorigi ADorigi marked this pull request as ready for review September 17, 2024 14:54
@ADorigi ADorigi requested a review from a team as a code owner September 17, 2024 14:54
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. new-feature Net-new feature labels Sep 17, 2024
@ADorigi ADorigi mentioned this pull request Sep 17, 2024
Open
TylerGillson
TylerGillson requested changes Sep 17, 2024
View reviewed changes
Copy link
Member

@TylerGillson TylerGillson left a comment

Choose a reason for hiding this comment

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

@ADorigi thanks!

This is great, but please relocate the constant to the validator core repo and add a helper there to construct a ctrl.Result given a slice of annotations. It can go under pkg/plugins. That way each plugin's controller can call the helper in the core repo to construct its result and we don't duplicate code.

Lastly, please do log the requeue delay so we know the annotation is working.

@ADorigi
Copy link
Author

ADorigi commented Sep 29, 2024 •
edited
Loading

@TylerGillson
I have shifted the functionality to validator core repo as advised: validator-labs/validator#422
I will update this PR to reflect the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerGillson TylerGillson TylerGillson requested changes

@arturshadnik arturshadnik Awaiting requested review from arturshadnik arturshadnik is a code owner automatically assigned from validator-labs/validator

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
new-feature Net-new feature size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🏗 Reconciliation frequency configuration via annotations
2 participants
@ADorigi @TylerGillson