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

Add ostree commit resolver for otk: otk-resolve-ostree-commit (COMPOSER-2349) #935

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Sep 16, 2024

New cmd for resolving ostree commits for otk.

The resolver is a simple call to ostree.Resolve() which handles resolving ostree input options to an ostree.CommitSpec.

The resolver returns the input as-is when the ref is a commit ID (there is nothing to resolve). This can happen when a user wants to pull an ostree commit by its ID rather than specifying a ref to resolve.

@achilleas-k achilleas-k changed the title Add ostree commit resolver for otk: otk-resolve-ostree-commit Add ostree commit resolver for otk: otk-resolve-ostree-commit (COMPOSER-2291) Sep 16, 2024
@achilleas-k achilleas-k changed the title Add ostree commit resolver for otk: otk-resolve-ostree-commit (COMPOSER-2291) Add ostree commit resolver for otk: otk-resolve-ostree-commit (COMPOSER-2349) Sep 16, 2024
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Just one little bit and maybe @mvo5 can chime in here but I think these outputs should live under a tree.const similar to the otk-gen-partition-table and osbuild-gen-depsolve-dnf4 externals that generate defines.

@achilleas-k
Copy link
Member Author

Just one little bit and maybe @mvo5 can chime in here but I think these outputs should live under a tree.const similar to the otk-gen-partition-table and osbuild-gen-depsolve-dnf4 externals that generate defines.

Ah, yes. Probably. Though, I'm not entirely sure what our rules and conventions are exactly.

Given the way we describe the Const object

// Const contains partition table data that is considered "constant",
// i.e. that should not be modified by the consumer as there may be
// inter-dependencies between the values

I think it's more appropriate for outputs from externals that will be fed back into another external.

The output from the resolver here is going to be used directly in stage options, so it's probably not necessary to mark them as const. Though maybe it's still a good convention to specify that modifying one or more might lead to inconsistencies.

@supakeen
Copy link
Member

supakeen commented Sep 16, 2024

Just one little bit and maybe @mvo5 can chime in here but I think these outputs should live under a tree.const similar to the otk-gen-partition-table and osbuild-gen-depsolve-dnf4 externals that generate defines.

Ah, yes. Probably. Though, I'm not entirely sure what our rules and conventions are exactly.

Given the way we describe the Const object

// Const contains partition table data that is considered "constant",
// i.e. that should not be modified by the consumer as there may be
// inter-dependencies between the values

I think it's more appropriate for outputs from externals that will be fed back into another external.

The output from the resolver here is going to be used directly in stage options, so it's probably not necessary to mark them as const. Though maybe it's still a good convention to specify that modifying one or more might lead to inconsistencies.

That is definitely a discussion we could have for now I think we can go with mashing it under const and then have a separate discussion about it when we have this one and the container resolving one in as well (we'll have 4 externals by then).

I mostly understood the adding of const to mean 'dont assign to this'.

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! I added some comments but feel free to ignore, mostly nitpicks/ideas/suggestions.

About the const vs inteneral. I'm happy to have a discussion here if needed, in my head it was mostly this heuristic:

  • const means that if a consumer modifies it things may fall apart/become inconsistent but const itself is fine to be used in an otk manifest and things under it are an API that we cannot easily break
  • internal means to be used only by internal tooling, i.e. matching producer/concsumer externals, anything under it is not an API and can change anytime as the consumer/produce pleases

cmd/otk-resolve-ostree-commit/main.go Show resolved Hide resolved
pkg/ostree/ostree.go Show resolved Hide resolved
cmd/otk-resolve-ostree-commit/main_test.go Outdated Show resolved Hide resolved
cmd/otk-resolve-ostree-commit/main_test.go Outdated Show resolved Hide resolved
cmd/otk-resolve-ostree-commit/main_test.go Show resolved Hide resolved
cmd/otk-resolve-ostree-commit/main_test.go Outdated Show resolved Hide resolved
Implement input and output types for new ostree commit resolver for otk.
The resolver is a simple call to ostree.Resolve() which handles
resolving ostree input options to an ostree.CommitSpec.

The resolver returns the input as-is when the ref is a commit ID (there
is nothing to resolve).
The condition would panic if an error was returned from the function
because 'subs' would be nil.  Also, the underlying error would be lost
if it wasn't nil.

Handle 'err != nil' separately to avoid dereferencing a nil 'subs' and
append the underlying error.

Keep the 'subs.Consume' nil check as well since the function can succeed
but not attach Consumer secrets when they're not available.  'subs'
should never be nil when the function succeeds.
Added tests for calling the resolver with a commit ID (see below) with
and without a URL as well as error tests.

Resolving by ID: When a commit ID is used in place of the ref, the
resolver returns the input commit ID as both the ref and the checksum.
No request goes to the server for this call.
@achilleas-k
Copy link
Member Author

The Output has been Consted.

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!

Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Needs the other comments resolved, awesome.

@supakeen supakeen added this pull request to the merge queue Sep 18, 2024
Merged via the queue into osbuild:main with commit 3c5acaf Sep 18, 2024
18 of 19 checks passed
@achilleas-k achilleas-k deleted the otk-external/ostree-resolver branch September 18, 2024 14:59
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.

4 participants