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

To add reconciliation time stamps #572

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Abiji-2020
Copy link
Contributor

@Abiji-2020 Abiji-2020 commented Sep 14, 2024

📑 Description

Updated the setStatus function in the ModuleReconciler to include the FinishedAt timestamp in the reconciliation status. The FinishedAt field now uses the current time, formatted as a string in RFC3339 format. Updated the custom JSON marshaler and unmarshaler for ReconciliationStatus to handle this timestamp field correctly. Fixes #430

  • Added FinishedAt field of type string to ReconciliationStatus.
  • Updated setStatus function to set FinishedAt to the current time in RFC3339 format.
  • Implemented custom JSON marshaler and unmarshaler for converting FinishedAt between time.Time and string.

✅ Checks

  • I have tested my code:
  • I have performed a self-review of my code:
    • Ensured setStatus function updates the FinishedAt timestamp correctly.
    • Verified that the custom JSON marshaler and unmarshaler handle the FinishedAt field as expected.

ℹ Additional context

  • Related Code Changes:
    • Updated the ReconciliationStatus struct to include a FinishedAt field.
    • Modified the setStatus function in the ModuleReconciler to set the FinishedAt field to the current time.
    • Implemented JSON marshaler and unmarshaler for proper handling of the FinishedAt timestamp.

… in the string formt and used them to store the history
@Abiji-2020 Abiji-2020 requested a review from a team as a code owner September 14, 2024 03:33
@petar-cvit
Copy link
Collaborator

@Abiji-2020 check the CI. Its failing on unused import

@Abiji-2020
Copy link
Contributor Author

Updates

There is a time package which was planned to use initally and then i have'nt used it so now i had removed it.

@Abiji-2020
Copy link
Contributor Author

How can i check for this checking as again tests in CI is failing?

Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

@Abiji-2020 thanks for the PR and the description of your changes! Left two comments, but could you also format the code? I see a lot of changes coming from different indentation

cyclops-ctrl/api/v1alpha1/module_types.go Outdated Show resolved Hide resolved
cyclops-ctrl/api/v1alpha1/module_types.go Outdated Show resolved Hide resolved
@Abiji-2020
Copy link
Contributor Author

The code was updated and the formatting was cleared

Copy link
Collaborator

@petar-cvit petar-cvit left a comment

Choose a reason for hiding this comment

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

@Abiji-2020 thanks, looks good! 🧡

Will merge after #582

@petar-cvit
Copy link
Collaborator

Hey @Abiji-2020, I tried running Cyclops from your branch one more time and found that Modules kept reconciling endlessly. The issue is that on each reconciliation, finishedAt is updated with a different timestamp, which then triggers a new reconciliation.

We need to change our reconciliation policy in order to include this PR and not trigger unnecessary reconciliations

@Abiji-2020
Copy link
Contributor Author

Hey @Abiji-2020, I tried running Cyclops from your branch one more time and found that Modules kept reconciling endlessly. The issue is that on each reconciliation, finishedAt is updated with a different timestamp, which then triggers a new reconciliation.

We need to change our reconciliation policy in order to include this PR and not trigger unnecessary reconciliations

so What can we do now. Like modifications.?

@petar-cvit
Copy link
Collaborator

@Abiji-2020 yeah, I will make a PR that will introduce event filter so reconciliation is not triggered on every module update. Will update here when that PR is merged and merge yours as well :)

@petar-cvit petar-cvit mentioned this pull request Sep 29, 2024
2 tasks
@Abiji-2020
Copy link
Contributor Author

@petar-cvit any updates on this?

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 reconciliation timestamp to the Module CRD
2 participants