Skip to content

feat(rockcraft-pack): support rockcraft test - #40

Merged
tigarmo merged 6 commits into
canonical:mainfrom
alesancor1:ROCKS-2151/support-rockcraft-test
Jun 24, 2025
Merged

feat(rockcraft-pack): support rockcraft test#40
tigarmo merged 6 commits into
canonical:mainfrom
alesancor1:ROCKS-2151/support-rockcraft-test

Conversation

@alesancor1

Copy link
Copy Markdown
Member

Summary

This PR adds support for rockcraft test command as part of the build workflow, since rockcraft test implicitly runs rockcraft pack before calling spread.

This behaviour prevents us from adding rockcraft test as another job inside OCI Factory Test-Rock workflow, as those rely on a previously packed rock by the Build workflow, where this action is used.

Ideally, if rockcraft test <rock-file> (or similar) becomes a reality, this logic can be moved to OCI Factory Test-Rock workflow.

@alesancor1 alesancor1 changed the title feat: support rockcraft test feat(rockcraft-pack): support rockcraft test May 26, 2025

@cjdcordeiro cjdcordeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pls re-request review when tests are passing

@alesancor1
alesancor1 requested a review from cjdcordeiro May 26, 2025 17:15
@cjdcordeiro
cjdcordeiro requested a review from a team May 27, 2025 05:27

@cjdcordeiro cjdcordeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was doing that change in my PR #39 , but since it might be dropped, you can also apply that change here: 468d0bb

@cjdcordeiro cjdcordeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM thanks!

Comment thread src/rockcraft-pack.ts Outdated
let rockcraft = 'rockcraft pack'
let rockcraftPackArgs = ''

if (tools.fileExists(`${this.projectRoot}/spread.yaml`)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this heuristic is enough. There are repositories that have a spread.yaml and use this action, but those spread tests are not meant for rockcraft test (they are regular spread tests that use gce).

Both this and checking whether "rockcraft test -h" works feel too magical; how about having users opt-in to this behavior with a new option on the action?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

another consideration is that even if a repository has rockcraft test-compliant tests, a failing test would fail the whole action, which is also surprising and a change in behavior.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added the opt-in flag, default to false. Check b8cb77d

run-rockcraft-pack-action:
strategy:
matrix:
os: [ubuntu-24.04, ubuntu-22.04, ubuntu-20.04]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how come?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

20.04 is end of standard support

@tigarmo tigarmo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks!

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