Add additionalMounts for GitHub Action Output Mount

[andar1an]

@stuartleeks

Beginning PR to add additional mounts required for GitHub Output in CI.

Recreating #187 work with a non-stale base.

WIP(DevContainerCliUpArgs - initial commit to add additional mounts for GitHub Action output (azdo-task/scripts/build-package.sh#L49->L64

There are a few deletions I did not make:

• Lines 188, 195, and 200 in common/src/dev-container-cli.ts.
• Lines 177 to 125 were formatted on save.

---

@stuartleeks update:
Fixes #180, #188, #195

[andar1an]

I am unsure how to go about testing this, some direction here would be wonderful. Thank you!

[stuartleeks]

In terms of testing, this is a good nudge to add something to the docs - thank you!

I just ran a test and discovered that there are a couple of things that have snuck through that prevent running tests in a fork and have created #220.

Once that is merged, you can go to the Actions tab for your fork and you will see the various workflows listed:

Click on CI (branch) as shown in the image above, and then click on the Run workflow button:

This will pop up some options. Select your PR branch, enter a number in the PR number box, and untick the run full tests box (the last bit is important):

This should do a run for most of the GitHub action tests. The Azure DevOps tests require an environment to be set up and configured which can be triggered by maintainers against a PR.

[andar1an]

@stuartleeks should I add back those deleted lines? I am not sure if it was my ide, or if a merge happened within the time I had synced and pushed.

[stuartleeks]

@stephenandary - yes, those lines are needed

[andar1an]

@stuartleeks in my tests I am getting hit by some permission, env token, and 429 errors, can't get past the first block. May require you to approve the test workflow to have tests run with correct secrets. Also, does a test need to be made for mounts? I did not do that.

[stuartleeks]

@stephenandary - #220 has been merged now, so if you merge that into your PR branch you should be able to use the steps above to run the subset of GH tests that don't need credentials on your fork (i.e. without needing someone else to trigger a test run)

In terms of tests, yes I think it would be good to add a test showing that GITHUB_OUTPUT is working from a runCmd

[andar1an]

@stuartleeks do you recall what the mount path should be from #187 (comment)?

[andar1an]

@stuartleeks do you recall what the mount path should be from #187 (comment)?

@stuartleeks, just bubbling this up again, as it will save me time having to figure this out if you know. Unless this is not a need to know for runCmd test.

I also merged main, I was 1 commit behind which explains those little changes that were required.

[stuartleeks]

There was some discussion around the GITHUB_OUTPUT handling in #188 - is that what you mean?

[andar1an]

Yes, that is perfect! I forgot about that one! Mount is /mnt/github/output, thank you!

[andar1an]

@stuartleeks based on the approach you highlighted in #188, I am unsure what tests are appropriate to modify.

I have added mounts in common/__tests__, (this does not test /mnt/github/output). I noticed that there are also github-tests. Should I be adding a mount or environment variable into the devcontainer.jsons for the various scenarios there? By the description of the solution approach, it seems this should be automatic; I am not understanding how this is currently automatic and how only the common test will cover it.

[stuartleeks]

The tests in common\__tests__ are unit tests, whereas the contents under github-tests are used by the integration tests in CI.
The folder is slightly misnamed now as the content is used by both GitHub workflow tests and Azure Devops pipeline tests! The GitHub workflow to exercise them is in .github/workflows/ci_common.yml and the Azure DevOps pipeline to use them is in .azure-devops/azure-pipelines.yml

[andar1an]

Thanks @stuartleeks! I think where I am getting confused a bit is to whether a user will need to add the GITHUB_OUTPUT mount in the devcontainer.json to use env variable, or if there is a way that does not require user input. I am going to go down the road of adding the mount this morning, and hopefully, this becomes more clear during that exercise.

[andar1an]

@stuartleeks ,
To highlight where I am currently at, with your proposed approach from #188

• mount the host's GITHUB_OUTPUT location to /mnt/github/output inside the dev container
• set the GITHUB_OUTPUT environment variable in the container to /mnt/github/output

Currently, in integration tests, I am thinking of adding a mount along the lines of:

"mounts": [
		// Keep command history 
		"source=build-args-bashhistory,target=/home/vscode/commandhistory",
		// Mount to utilize GITHUB_OUTPUTS
		"source=/home/runner/work/${REPO}/${REPO}/.github/${GITHUB_ACTION}/${STEP_NAME}/output, target=/mnt/github/output"
],

I am not sure if this is the correct approach so trying to document in PR as I go. I imagine I would need to update the DOCKERFILE to reflect the GITHUB_OUTPUT env variable. I am hoping you know how I can do this without the env.variables as I don't think they will work here. Will a user also need to do both of these things?

Does this make sense to you?

[stuartleeks]

I imagined that the action code would determine the values for GITHUB_OUTPUT etc on the host and automatically add that as an additional mount when invoking the CLI, at the same time also setting the GITHUB_OUTPUT environment variable for the container to point to the mounted location. And then to do this for GITHUB_STATE etc.

It has occurred to me that anyone using this action and setting outputs/environment variables etc will be using the same approach that the action uses (which is now deprecated). We should make sure we get this change in soon to allow people chance to migrate to the new approach prior to the current approach being disabled. We're also seeing an quite a lot of failures in CI builds that seem to stem from the existing set-output approach not always working (I recently added some additional output to confirm this). I'm hoping that getting this change in will also improve the build reliability.

[andar1an]

@stuartleeks, to make sure I understand, I should not worry about runArgs and buildArgs tests, and just update the .github/workflows/ci_common.yml and .azure-devops/azure-pipelines.yml actions? Currently that is any action step that may contain:

echo "VERSION_MAJOR=${VERSION_MAJOR}"
echo "VERSION_MINOR=${VERSION_MINOR}"
echo "VERSION_PATCH=${VERSION_PATCH}"

[stuartleeks]

As discussed in one of the earlier comments, I pushed a couple of commits to add the mounts/env vars for GITHUB_OUTPUT and that resolved the errors you were seeing around quoting $GITHUB_OUTPUT in the script.

I also pushed the changes from #221 here, so I've closed that PR in favour of this one. I pushed this to the pr-219 branch in the upstream repo to kick of the tests and found a few more action upgrades that were needed to remove some more warnings around NodeJS versions, set-state etc. I've pushed those and this run is a clean run 🎉

Thanks for all your work on this @stephenandary !

[andar1an]

@stuartleeks this is so wonderful! Thank you for helping me learn. It would have taken me a while to figure out those additional changes and I'm grateful you could take this over the finish line. It was an enjoyable experience to work on, and thats all you! P.S. this was my first Open Source PR ever haha, going to save this one as a memento!

[stuartleeks]

@stephenandary - you are welcome. Thanks again for your work on this, and here's to many more OSS PRs 😁

[chrmarti]

LGTM!