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

feat: add ability to manually sync layers #321

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

Conversation

LucasMrqes
Copy link
Collaborator

@LucasMrqes LucasMrqes commented Aug 30, 2024

image image image

@Alan-pad
Copy link
Member

Alan-pad commented Sep 2, 2024

@LucasMrqes If you create a TerraformRun directly that would be easier I think and should still work, you'd have to make sure there's no other TerraformRun running but apart from that it should be straightforward to implement plan/apply buttons

@LucasMrqes
Copy link
Collaborator Author

LucasMrqes commented Sep 6, 2024

@LucasMrqes If you create a TerraformRun directly that would be easier I think and should still work, you'd have to make sure there's no other TerraformRun running but apart from that it should be straightforward to implement plan/apply buttons

I think using the controller to generate the run is a better way. I tried to create the run resource with the server and encountered the following issues:

  • The status of the layer is not updated, thus making the layer status + run history incoherent and messing with how the server finds the last run in its /layers path. (unless we duplicate this logic in the server as well)
  • Not using the controller to generate runs makes it complicated to debug using the manual sync Button, as the run creation logic is somewhat duplicated and the button would bypass the ususal flow of creating run/pods with Burrito
  • Overall, the required logic is way more complicated

wdyt @Alan-pad ?

@Alan-pad
Copy link
Member

Alan-pad commented Sep 9, 2024

I think we can adapt the controller logic so the new runs are picked up

@LucasMrqes
Copy link
Collaborator Author

Implementing this feature with a tfrun creation requires us to rework the logic of the layer controller. This is something that we plan to do, but since this feature is requested a lot by Burrito users, we will merge this as is and work on a rework on future versions of Burrito.

Copy link
Member

@corrieriluca corrieriluca left a comment

Choose a reason for hiding this comment

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

Great feature! 👏

Comment on lines +259 to +263
// Remove the annotation to avoid running the sync again
err := annotations.Remove(context.Background(), r.Client, t, annotations.SyncNow)
if err != nil {
log.Errorf("Failed to remove annotation %s from layer %s: %s", annotations.SyncNow, t.Name, err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Removing the annotation right away is causing another reconciliation right?
Does the layer end in a NoSyncScheduled status just a few seconds later? Does it impact the Server API in its SyncLayerHandler ?

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