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

chore: update witness.yml with workingdir input #16

Open
wants to merge 3 commits into
base: reusable-workflow
Choose a base branch
from

Conversation

kriscoleman
Copy link
Contributor

changes to get workingdir working with john's reusable workflow branch

@jkjell we have this and one other PR on your reusable workflow branch. Total that's 3 prs not merged to main that we're using in one place or another. We should get those all merged before we lose track.

@kriscoleman kriscoleman mentioned this pull request Jan 13, 2025
@kriscoleman kriscoleman force-pushed the workingdir-input branch 4 times, most recently from 306b71b to e229e82 Compare January 15, 2025 19:36
@kriscoleman kriscoleman marked this pull request as draft January 15, 2025 19:38
@kriscoleman
Copy link
Contributor Author

@georgesdugue and I found more issues with this. Because of the conditional on pull_request, it changes how the process runs after merge, and after merge things broke down.

working through some more fixes

@kriscoleman kriscoleman force-pushed the workingdir-input branch 3 times, most recently from c3d9089 to 6bea697 Compare January 15, 2025 19:54
Added a new 'workingdir' input to the GitHub Actions workflow. This allows for specifying the working directory when running commands. The changes include:
- Added 'workingdir' as an optional input in the workflow_call section.
- Updated testifysec/witness-run-action usage to include 'workingdir'.
- Set 'working-directory' for run commands when pull_request is true.
This enhancement provides more flexibility in configuring workflows, especially when dealing with projects that have complex structures or require specific execution contexts.
@kriscoleman
Copy link
Contributor Author

kriscoleman commented Jan 15, 2025

worked through the issues

  • ncc wasn't in package.json so couldn't build
  • fixed conditionals for workingdir when pull_request == false.
  • witness-install-dir input had a default of ./ but there was a conditional checking for ./ and when we updated it to use workingdir, the conditional would never hit second condition

@kriscoleman kriscoleman marked this pull request as ready for review January 15, 2025 20:19
Copy link
Contributor

Choose a reason for hiding this comment

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

Was anything actually changed in this file other than whitespace/formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the default value ./ from witness-install-dir because it was already being handled in code.
I also changed how it was being handled so now it works with or without workingdir, you can see this being handled in index.js

@@ -154,22 +136,22 @@ async function run() {

if (trace) cmd.push(`--trace=${trace}`);
if (outfile) cmd.push(`--outfile=${outfile}`);
core.info("Running in directory " + process.env.GITHUB_WORKSPACE);
core.info('Running in directory ' + fullWorkspacePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these the only "real" changes in this file?

Copy link
Contributor Author

@kriscoleman kriscoleman Jan 15, 2025

Choose a reason for hiding this comment

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

this and lines 12-14

I found workingdir was defined but wasn't being used.

default: 'https://archivista.testifysec.io'
required: false
type: string
workingdir:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh @jkjell I also had to add workingdir here, it was missing here too

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.

2 participants