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

osbuild-worker: use the new ostree resolver API (HMS-4773) #4412

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

lzap
Copy link
Contributor

@lzap lzap commented Oct 15, 2024

For Edge Pulp migration we need the very same thing as repositories (curl) but for ostree input.

croissanne
croissanne previously approved these changes Oct 15, 2024
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

lgtm! thanks

cmd/osbuild-worker/jobimpl-osbuild.go Outdated Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 15, 2024

Thanks, amended. For the record, these variables will be used in osbuild input. For commit resolution that happens in composer, I am working on a different patch that will read the config file directly:

osbuild/images#975

Composer part of this will come as another PR.

@lzap
Copy link
Contributor Author

lzap commented Oct 16, 2024

RPM packaging (unrelated?) failures.

@lzap lzap changed the title osbuild-worker: set ostree input MTLS vars osbuild-worker: use the new ostree resolver API Oct 22, 2024
@lzap
Copy link
Contributor Author

lzap commented Oct 22, 2024

So @achilleas-k wanted me to break the API and clean it up, RHSM/secrets are no longer used, the resolve function now takes a new struct that provides MTLS information.

Tests will fail until change in images is merged.

go.mod Outdated Show resolved Hide resolved
internal/weldr/api.go Outdated Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Oct 31, 2024

Amended vastly simplified version after discussion with @achilleas-k and @croissanne

@lzap lzap force-pushed the ostree-env branch 2 times, most recently from b5c975e to 9c9bc3f Compare November 4, 2024 15:43
@lzap lzap marked this pull request as ready for review November 4, 2024 15:43
@lzap
Copy link
Contributor Author

lzap commented Nov 4, 2024

Amended the images library version bump to 0.96 which contains the required change.

@lzap lzap force-pushed the ostree-env branch 2 times, most recently from 645a329 to 73b0cd4 Compare November 4, 2024 15:48
@lzap
Copy link
Contributor Author

lzap commented Nov 4, 2024

There are errors which might be related to this work:

Error: internal/weldr/upload.go:286:15: undefined: distro.BOOT_HYBRID
  Error: internal/weldr/upload.go:288:15: undefined: distro.BOOT_UEFI
  Error: internal/weldr/upload.go:290:15: undefined: distro.BOOT_LEGACY (typecheck)

I see a related change made by @achilleas-k so how are we gonna approach this?

@lzap
Copy link
Contributor Author

lzap commented Nov 4, 2024

Squashing the boot type minor refactoring.

@lzap
Copy link
Contributor Author

lzap commented Nov 4, 2024

Note for myself: needs to be rebased on top of #4412

@achilleas-k
Copy link
Member

Note for myself: needs to be rebased on top of #4412

Correction: #4448

achilleas-k
achilleas-k previously approved these changes Nov 5, 2024
@achilleas-k
Copy link
Member

So this doesn't change anything about the request right? Composer doesn't seem to be configuring anything in the resolve job nor the manifest creation yet?
The PR right now is pulling in the new images library and making it work but it's not reading the option to enable the certs from anywhere.

Is that coming in this PR or are you leaving it for a follow-up? In any case, can you update the PR title and description accordingly?

@lzap
Copy link
Contributor Author

lzap commented Nov 5, 2024

The PR right now is pulling in the new images library and making it work but it's not reading the option to enable the certs from anywhere.

Correct me if I am wrong, but I thought this osbuild-worker.go hunk does the change. It reads the configuration and applies it for all requests.

I am not planning any follow-up, my intention is to automatically use MTLS configuration for all ostree requests in hosted code path. On-prem ignores MTLS, this is where no configuration is read.

@lzap lzap requested a review from croissanne November 5, 2024 10:27
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

lgtm overall, would be slightly easier to review if the images update commit would be separate.

cmd/osbuild-worker/jobimpl-ostree-resolve.go Show resolved Hide resolved
@lzap
Copy link
Contributor Author

lzap commented Nov 5, 2024

Tests looks green, yay.

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

@achilleas-k achilleas-k enabled auto-merge (rebase) November 7, 2024 15:17
@achilleas-k achilleas-k merged commit 64f4790 into osbuild:main Nov 7, 2024
47 of 50 checks passed
@ezr-ondrej ezr-ondrej changed the title osbuild-worker: use the new ostree resolver API osbuild-worker: use the new ostree resolver API (HMS-4773) Nov 7, 2024
@lzap lzap deleted the ostree-env branch November 7, 2024 17:07
@lzap
Copy link
Contributor Author

lzap commented Nov 7, 2024

Thank you so much!

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