Skip to content

feat(rockcraft-pack): accept env-file input - #39

Draft
cjdcordeiro wants to merge 5 commits into
mainfrom
feat/rockcraft-accept-env
Draft

feat(rockcraft-pack): accept env-file input#39
cjdcordeiro wants to merge 5 commits into
mainfrom
feat/rockcraft-accept-env

Conversation

@cjdcordeiro

Copy link
Copy Markdown
Contributor

Let the rockcraft-pack action accept an optional input to specify the execution environment to be sourced before running Rockcraft.

@alesancor1 alesancor1 left a comment

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.

LGTM, thanks!

@cjdcordeiro
cjdcordeiro requested review from a team and sergiusens May 22, 2025 08:55
Comment thread rockcraft-pack/action.yml Outdated
@mr-cal
mr-cal requested a review from a team May 23, 2025 15:11

@lengau lengau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Giving this a tentative yes because it seems fine, but I have an entire lack of confidence in my typescript knowledge.

if (this.envFile) {
sourceCmd = `source ${this.envFile} && `;
}
const bashCommand = `${sourceCmd}sudo --preserve-env --user ${shellUser()} ${rockcraft}`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is unrelated to the PR, but this is an interesting example of where switching to sudo-rs could cause issues.

Co-authored-by: Callahan <callahan.kovacs@canonical.com>
@cjdcordeiro
cjdcordeiro marked this pull request as draft May 23, 2025 15:43
@cmatsuoka
cmatsuoka requested review from a team and Copilot May 23, 2025 16:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables the rockcraft-pack action to accept an optional env-file input, which is sourced before running the Rockcraft pack command.

  • Adds a new envFile property throughout the builder, action, and tests
  • Updates exec.exec to run via bash -c, optionally prefixing with source <env-file>
  • Extends action metadata and adjusts workflow tests accordingly

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/rockcraft-pack.test.ts Add envFile input to test cases and update expected execMock calls
src/rockcraft-pack.ts Introduce envFile in options and source the file before pack command
src/rockcraft-pack-action.ts Retrieve env-file input and forward to RockcraftBuilder
rockcraft-pack/action.yml Declare new env-file input in action metadata
.github/workflows/test.yml Update CI matrix by removing Ubuntu 20.04
Comments suppressed due to low confidence (1)

.github/workflows/test.yml:35

  • Dropping Ubuntu 20.04 from the matrix reduces platform coverage; consider retaining it if still supported.
os: [ubuntu-24.04, ubuntu-22.04]

Comment thread src/rockcraft-pack.ts Outdated
Comment thread src/rockcraft-pack.ts
Comment thread rockcraft-pack/action.yml
cjdcordeiro and others added 2 commits May 26, 2025 12:11
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@tigarmo

tigarmo commented May 27, 2025

Copy link
Copy Markdown
Contributor

@cjdcordeiro I'm fine with this PR, but it's in Draft and has a missing test. Just pointing this out in case it dropped off your radar

@cjdcordeiro

Copy link
Copy Markdown
Contributor Author

Thanks @tigarmo .

I moved this to Draft because after an internal discussion we had. Let me ping you there and feel free to close this PR if you agree that this is not needed after all

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.

6 participants