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

Switch to oaas / osbuild-worker-executor (HMS-4097) #4184

Merged
merged 11 commits into from
Jun 12, 2024

Conversation

croissanne
Copy link
Member

@croissanne croissanne commented Jun 3, 2024

This simplifies the way the worker-executor, there's only 3 http calls happening: wait until ready, post for a build, get for the results.

@croissanne croissanne force-pushed the oaas branch 10 times, most recently from 25298d5 to 304f7a2 Compare June 3, 2024 20:34
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Just as a quick drive-by comment, it might be nice if the first commit with the copy of oaas also points to the original repo url in the description (that will then be archived). Just so that the git history is complete.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I'm very excited! I did a quick runthrough and added some small sugestions/ideas, mostly drive-by. Super happy to help out here too, I would love for example to write some tests for runner-impl-aws-ec2.go :)

internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
internal/osbuildexecutor/runner-impl-aws-ec2.go Outdated Show resolved Hide resolved
@croissanne croissanne force-pushed the oaas branch 2 times, most recently from 63163c3 to 0661cbc Compare June 6, 2024 10:27
mvo5
mvo5 previously approved these changes Jun 7, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you! This looks really nice!

internal/osbuildexecutor/export_test.go Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Jun 7, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Still looks good (after the rebase)

croissanne and others added 10 commits June 7, 2024 18:58
In RunOsbuild return more verbose errors where the error doesn't
originate from one of the local helper functions.
This commit fixes a warning from tar that the archive cannot contain
itself. It also makes any tar output a warning (maybe even an error?)
as we do not expect anything from the tar command. The test is updated
to also check this.
The tar file from the `osbuild-worker-executor` is potentially
tainted. Ensure we validate and only extract if it harmless.
@croissanne
Copy link
Member Author

@mvo5

-       if err := run(ctx, os.Args[1:], os.Getenv, logger); err != nil {
+       if err := run(ctx, os.Args, os.Getenv, logger); err != nil {
                fmt.Fprintf(os.Stderr, "%s\n", err)
                os.Exit(1)
        }

got applied in the rebase again, reverted that, not sure where ._.

Otherwise the client will try to fetch the output archive before the
build output is archived on the worker-executor side.
@croissanne croissanne requested a review from mvo5 June 12, 2024 08:50
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

@croissanne croissanne enabled auto-merge (rebase) June 12, 2024 09:35
@croissanne croissanne disabled auto-merge June 12, 2024 09:36
@croissanne croissanne merged commit 8a5f486 into osbuild:main Jun 12, 2024
40 of 67 checks passed
@croissanne croissanne deleted the oaas branch June 12, 2024 09:36
@croissanne croissanne changed the title Switch to oaas / osbuild-worker-executor Switch to oaas / osbuild-worker-executor (HMS-4097) Jun 21, 2024
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.

2 participants