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

fly: add background option to execute command #8856

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

KoltesDigital
Copy link
Contributor

@KoltesDigital KoltesDigital commented Nov 21, 2023

Changes proposed by this PR

This patch adds a new -b/--background option to fly execute. This option tells fly to create the build, and exit just before watching the logs. It also means that outputs can't be retrieved. Instead, the build ID is printed, and logs can then be followed with fly watch.

  • added feature with acceptance test
  • updated documentation
  • left trail in contributors

Rationale

I self-host Gitea and Drone. The application I'm making also needs some worker pool to spawn short-lived processes, and I thought it would be cool to only have one generic job framework to do both. But Drone is not flexible enough for that; besides it has been bought and shut down. I don't want to use frameworks that would lock me in K8S.

So, let's give Concourse a try! CICD works fine, however my jobs are triggered out of nowhere so I can't rely on resource checks to launch them. Although... maybe a custom resource type listing pending jobs with version: every could have somewhat worked, but can't be parallelized... Anyway, I feel spawning one-off tasks fits more to the idea.

I've seen on some pages that ATC's REST API is private, and on some other pages it's somewhat private only because it hasn't been documented yet. Well, my app will communicate through the API for sure, regardless of its privacy! I can't rely on spawning a process and waiting for it, my app can be restarted anytime, and jobs should not depend of that. So I prefer to independently spawn a job, and retrieve its logs.

I thought this workflow could be useful to CLI users too. Build ID is printed so that spawner process can use it (works with Bash and PowerShell).

Note that I don't need to fetch outputs back (yet), but CLI users could benefit from the complete split workflow: fly execute --background + fly watch + to-be-done fly outputs.

Notes to reviewer

I don't master flyIn. It would have been better to test afterwards that fly watch <buildID> would have yield the same outputs as the preceding test, but then test times out. Maybe it's expected that watch keeps running if it's spawned while the build is running too, but I vaguely recall that it exits as soon as the build is completed.

Besides I'm new to Go, so I tried to fit in the code style, but have no idea how idiomatic it is.

Alternative name could have been -d/--daemon (like docker run does), but it implies the process is long-lasting, while it shouldn't.

Release Note

  • fly execute gets new -b/--background option to create builds without watching them.

@KoltesDigital KoltesDigital requested a review from a team as a code owner November 21, 2023 14:18
@taylorsilva
Copy link
Member

however my jobs are triggered out of nowhere so I can't rely on resource checks to launch them

This feels like a pipeline design problem. Are you still facing this pain? Could you share more details on how your pipelines are structured?

@KoltesDigital
Copy link
Contributor Author

KoltesDigital commented Mar 29, 2024

@taylorsilva Well I meant, it's not out of nowhere, that's very deterministic: every time users click on the button. However, now that I have more experience with Concourse, I would model my need with a queue resource. I.e. when a user wants to run a job, instead of instructing Concourse to run it, the request is pushed into a queue, which will appear as a version to Concourse. A pipeline would pop and execute each request. This has the benefit of letting Concourse take care of limiting parallel jobs and other stuffs. The drawback, rather an extra effort, is that I'll have to monitor all build creations and match them back to the original requests. Regarding resource checks, a webhook trigger would force almost-immediate processing.

Still, I think that the flag could be useful. Usually log CLIs have a follow flag. In your case you're already following logs, but I don't understand why not-following wouldn't be allowed.

Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

Still, I think that the flag could be useful. Usually log CLIs have a follow flag. In your case you're already following logs, but I don't understand why not-following wouldn't be allowed.

Agreed, I can't think of a reason to not have this feature and am fine with merging it in. See below for feedback about the command output with this flag.

fly/commands/execute.go Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Giroux <giroux.jo@gmail.com>
@taylorsilva
Copy link
Member

Looks like there's an issue with the watjs tests. Seeing it on master too. We'll have to look into that to unblock.

@taylorsilva taylorsilva merged commit 2a7bb08 into concourse:master Apr 2, 2024
11 checks passed
@taylorsilva
Copy link
Member

Thanks for the PR @KoltesDigital!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants