Set-Output/Set-State Deprecation Update2

[andar1an]

Remaking #181

Update to handle change for set-output and save-state syntax as per: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

I hope this quote placement is correct. I apologize I don't know how to test locally before pushing.

Why is it being echoed instead of output without echo?

[stuartleeks]

/test fa4a64b

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/devcontainers/ci/actions/runs/3719060344

(in response to this comment from @stuartleeks)

[stuartleeks]

@stephenandary - thanks for pushing on this. I was initially confused why the changes you made were failing and the error message didn't seem helpful at all!

I just spent some time going through this and realised that that script is run in the dev container, so the GITHUB_OUTPUT environment variable wasn't set. I just added that and kicked off a task but realised afterwards that GITHUB_OUTPUT will be pointing to a path that doesn't exist inside the dev container, so will need an additional mount point. This isn't currently supported by the action (and I don't think it is supported by the CLI yet either), so there are a few steps needed to enable this.

[andar1an]

@stuartleeks I didn't think to grep other repositories! The only references I found in the CI repo were those 2 lines. I will take a peak at the cli repo now.

[andar1an]

@microsoft-github-policy-service agree company="Jammin Music, Inc."

[andar1an]

For the CLI I think it can handle multiple mount-points based on: devContainersSpecCLI.ts#L112

Therefore do you think it is possible to just add the additional mounts in the actions?

Per #188

• 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

If you could point me to where I should look to make mount updates in this repo that would be great. Also, where do you see what you discovered (I would love to understand :) )

It appears that you do most of the action logic with TypeScript and I am trying to determine how you are currently setting state or output in actions, and mounting directories. I am thinking the dist directory is the most relevant and am looking at the following files.

• docker.d.ts
• envvars.d.ts
• config.d.ts
• index.js

If you can point me when you have a moment that would be great. I am going to leave it for now as it is a lot to dig through haha.

[stuartleeks]

As you mentioned ,the action is written in TypeScript. The dist folder is the output from the TypeScript compilation step.

This repo contains the code for both a GitHub Action and an Azure DevOps Task. Common code between the two is in the common folder.

My starting point would probably be to modify the DevContainerCliUpArgs in the common code to include additional mounts.

Then in the GitHub action code, add the logic to set mounts for the files listed in the Handle GITHUB_OUTPUT issue, and set the env vars for the dev container to point to the location of the mount inside the container.

Hope that makes sense?

[andar1an]

Cool! I am working on something else during the week, but I can try to get to that this weekend

[andar1an]

I haven't gotten to this yet, I didn't realize the date when I said "I can try to get to it on the weekend" haha. I have not forgotten about it.

[stuartleeks]

@stephenandary - just checking back, is this still something you plan to work on?

[andar1an]

@stephenandary - just checking back, is this still something you plan to work on?

@stuartleeks I was looking at this over the weekend and am not too sure how to do what is needed. If I could work with someone, or if it is easier for someone else with experience to do this that will be faster.

For the first part would it be something like(https://github.com/devcontainers/ci/blob/main/common/src/dev-container-cli.ts#L182):

export interface DevContainerCliUpArgs {
  workspaceFolder: string;
  additionalCacheFroms?: string[];
  skipContainerUserIdUpdate?: boolean;
  userDataFolder?: string;
  additionalMounts?:string[];
}
async function devContainerUp(
  args: DevContainerCliUpArgs,
  log: (data: string) => void,
): Promise<DevContainerCliUpResult | DevContainerCliError> {
  const commandArgs: string[] = [
    'up',
    '--workspace-folder',
    args.workspaceFolder,
  ];
  if (args.additionalCacheFroms) {
    args.additionalCacheFroms.forEach(cacheFrom =>
      commandArgs.push('--cache-from', cacheFrom),
    );
  }
  if (args.userDataFolder) {
    commandArgs.push("--user-data-folder", args.userDataFolder);
  }
  if (args.skipContainerUserIdUpdate) {
    commandArgs.push('--update-remote-user-uid-default', 'off');
  }
  if (args.additionalMounts) {
    args.additionalMounts.forEach(cacheFrom =>
      commandArgs.push('--additional-mount', additionalMounts),
    );
  }
  return await runSpecCli<DevContainerCliUpResult>({
    args: commandArgs,
    log,
    env: {DOCKER_BUILDKIT: '1', COMPOSE_DOCKER_CLI_BUILD: '1'},
  });
}

For part 2 would it be something like(https://github.com/devcontainers/ci/blob/main/github-action/src/main.ts#L111):

if (runCommand) {
			const upResult = await core.group('🏃 start container', async () => {
				const args: DevContainerCliUpArgs = {
					workspaceFolder,
					additionalCacheFroms: cacheFrom,
					skipContainerUserIdUpdate,
					userDataFolder,
					additionalMounts: additionalMount,
				};
				const result = await devcontainer.up(args, log);
				if (result.outcome !== 'success') {
					core.error(
						`Dev container up failed: ${result.message} (exit code: ${result.code})\n${result.description}`,
					);
					core.setFailed(result.message);
				}
				return result;
			});
			if (upResult.outcome !== 'success') {
				return;
			}

I am not sure if the language has to align with "mount" here: https://github.com/devcontainers/cli/blob/main/src/spec-node/devContainersSpecCLI.ts#L112. E.g. instead of --additional-mount, should it be --mount. I may be way off, but this is where I was at

[andar1an]

@stuartleeks, do you think I am on the right track for the above? Would it be worth adding more time?

[stuartleeks]

@stephenandary - I'm currently working to fix up some PR build issues. After that, getting this resolved is fairly high on my list.

To answer the question about the CLI args, you would need to use --mount to pass the additional mounts in

[andar1an]

@stephenandary - I'm currently working to fix up some PR build issues. After that, getting this resolved is fairly high on my list.

To answer the question about the CLI args, you would need to use --mount to pass the additional mounts in

Awesome, and thank you. I would still like to help get this done as long as I am not negatively impacting timeline. I apologize, I have never worked with TS before, so when I have time to look at it I am not very quick. I will try to spend more time soon.

[andar1an]

@stuartleeks I am almost done working on something else that has been taking the bulk of my time. I would like to put this in my calendar for later this week to work on. Do you think a new PR should be used? I am not sure how stale this has become yet.

[stuartleeks]

@stephenandary - great news!

Whichever option you prefer is fine. If you continue on this branch, it would be worth merging main into it. You'll get merge conflicts on the generated JS code, but running the local build script will iron that out.

[stuartleeks]

@stephenandary - if you haven't already started work on this, you may want to branch off latest main as I've just pushed through some changes that mean that the compiled JS is no longer a part of the repo. Hopefully this will simplify the process for contributing going forwards and reduce merge conflicts between PRs :-)

I've also added some additional logging in the build, and there seems to be an issue with the set-output values not working reliably. Since that approach is deprecated, I'm keen to see what we've discussed here implemented so that we can move off that approach, and hopefully improve the reliability of the builds/releases. If you don't think you will get time to send a PR then let me know 😄

[andar1an]

@stephenandary - if you haven't already started work on this, you may want to branch off latest main as I've just pushed through some changes that mean that the compiled JS is no longer a part of the repo. Hopefully this will simplify the process for contributing going forwards and reduce merge conflicts between PRs :-)

I've also added some additional logging in the build, and there seems to be an issue with the set-output values not working reliably. Since that approach is deprecated, I'm keen to see what we've discussed here implemented so that we can move off that approach, and hopefully improve the reliability of the builds/releases. If you don't think you will get time to send a PR then let me know 😄

@stuartleeks I was doing some tangental work to dive back into this yesterday, but when I try to build devcontainer and run it I am unable to start a terminal in vscode and get a pty timeout. I found an issue related to it but changing versions like others doesn't work for me. It is making it a challenge to get to a point where I can test changes, though I don't think it affects CI and only vscode so I can just keep running CI builds.

I am going to work on what has been discussed today. If you have time for a synchronous conversation to help me get my bearings again that would be wonderful. I will close and tag this PR and start a new one with this PR tagged in it so that I don't need to show a merge in commit.

[stuartleeks]

@stephenandary - are you in the devcontainers community slack channel? (#180 (comment))

[andar1an]

I just requested, I have not joined yet because I don't think I have slack connect, I just use free tier.

[stuartleeks]

No worries - feel free to email me: stuartle@microsoft.com

[andar1an]

Thanks @stuartleeks. I just ran into a router hiccup, and I think you are in a different time zone so I will do my best to get as far as I can by Monday and then email.

[stuartleeks]

What timezone are you?

[andar1an]

Eastern, I usually see your posts come in earlier so guessing you are in EU? Or you are just a morning person 🤣

[stuartleeks]

Yeah, I'm in UK but my team is spread across timezones (including east and west US!)

[andar1an]

Closing this PR to create new one that is not stale.