Skip to content

cmd/rofl/build: Validate service volume source#494

Merged
abukosek merged 2 commits into
masterfrom
andrej/feature/build-rofl-check-volumes
Jun 20, 2025
Merged

cmd/rofl/build: Validate service volume source#494
abukosek merged 2 commits into
masterfrom
andrej/feature/build-rofl-check-volumes

Conversation

@abukosek
Copy link
Copy Markdown
Contributor

Closes #462.

@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 12, 2025

Deploy Preview for oasisprotocol-cli canceled.

Name Link
🔨 Latest commit 0bacfbb
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-cli/deploys/6853dcd9f1404700079ccf86

@abukosek abukosek force-pushed the andrej/feature/build-rofl-check-volumes branch from 80618f6 to 0ed5533 Compare June 12, 2025 13:29
@abukosek abukosek changed the title cmd/rofl/build: Validate service volume mountpoint cmd/rofl/build: Validate service volume source Jun 12, 2025
@abukosek abukosek force-pushed the andrej/feature/build-rofl-check-volumes branch 3 times, most recently from ac2eb42 to 8c4acf0 Compare June 12, 2025 13:41
@abukosek abukosek marked this pull request as ready for review June 12, 2025 13:44
@abukosek abukosek requested a review from matevz June 12, 2025 13:44
Comment thread cmd/rofl/build/container.go Outdated
}

var ok bool
for _, v := range proj.Volumes {
Copy link
Copy Markdown
Member

@matevz matevz Jun 13, 2025

Choose a reason for hiding this comment

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

Would you need to check proj.Volumes too for storage and rofl-appd.sock? Or maybe I didn't get what this check does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This checks if the volume is defined in the top-level volumes config.

Comment thread cmd/rofl/build/container.go Outdated

// tdxBuildContainer builds a TDX-based container ROFL app.
func tdxBuildContainer(
func tdxBuildContainer( //nolint:gocyclo
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe split into multiple validation functions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Splitting validation would also be useful for adding a validation command. For example, for the ROFL dashboard backend, I think it would be useful having something like oasis rofl validate (or oasis rofl build validate), which just does the (quick) validations without actually building the ROFL App. That way, the API will be able to reject invalid manifests quickly before starting the longer-running job to build them.

If you agree to do this, let me know so I open an issue for it :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good :) I'll move the validation into its own function and the rest can be done in a separate PR later.

@kostko
Copy link
Copy Markdown
Member

kostko commented Jun 13, 2025

Also there should be a way to force build anyway.

@ptrus ptrus mentioned this pull request Jun 17, 2025
@abukosek abukosek force-pushed the andrej/feature/build-rofl-check-volumes branch from 8c4acf0 to 5790331 Compare June 17, 2025 09:07
@abukosek abukosek requested a review from matevz June 19, 2025 07:47
Comment thread cmd/rofl/build/validate.go Outdated
return true
}

if strings.HasPrefix(src, "/storage") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/storage2/mydir should also fail.

Suggested change
if strings.HasPrefix(src, "/storage") {
if strings.HasPrefix(src, "/storage/") {

Comment thread cmd/rofl/build/container.go Outdated
image = digestBits[0]
tag = digestBits[1]
if err := validateComposeFile(artifacts[artifactContainerCompose], manifest); err != nil {
if common.IsForce() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use common.CheckForceErr() for standardized force flag check and output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed :)

@abukosek abukosek force-pushed the andrej/feature/build-rofl-check-volumes branch from e41e778 to 0bacfbb Compare June 19, 2025 09:48
Copy link
Copy Markdown
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

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

LGTM.

Let's open another PR for rofl.yaml and compose.yaml validation.

@abukosek abukosek merged commit d917159 into master Jun 20, 2025
4 checks passed
@abukosek abukosek deleted the andrej/feature/build-rofl-check-volumes branch June 20, 2025 09:35
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.

rofl build: Should validate "volumes" in compose.yaml

4 participants