Skip to content

Conversation

@robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented Oct 13, 2020

Instead of building the runner locally we should probably standardise on building it within actions to get a consistent environment. This PR introduces a workflow that just builds the runner and upload the artefacts.

I'm intending this currently state as just a starting point. A couple of open questions I have are:

  • Should this run on push or just workflow_dispatch? I only added push so I could test it before merging to the default branch.
  • Should we merge this with split.yml? The primary use case for this is when doing a release, but it could be useful to be able to run separatetly too.

@rneatherway

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@rneatherway
Copy link
Contributor

I suggest just using workflow_dispatch and adding the ability for it to upload the artefacts directly to the specified release like this:

- name: Upload Platform Package
uses: actions/upload-release-asset@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
upload_url: ${{ steps.save_url.outputs.upload_url }}
asset_path: ./codeql-bundle-${{matrix.platform}}.tar.gz
asset_name: codeql-bundle-${{matrix.platform}}.tar.gz
asset_content_type: application/tar+gzip

Keep it separate though as split.yml will be able to go away when we start preparing those platform-specific bundles ahead of time.

@rneatherway rneatherway force-pushed the robertbrignull/runner_workflow branch from 1c7f052 to d91d2d2 Compare October 20, 2020 10:00
@rneatherway rneatherway force-pushed the robertbrignull/runner_workflow branch from 7636ec9 to c1e2c53 Compare October 20, 2020 10:58
@rneatherway
Copy link
Contributor

I've tested this using the on: push trigger and a hardcoded RELEASE_TAG, which I then removed in the last commit.

Once this is merged I will delete the test release.

Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

One small change, but otherwise it looks good to me.

extension: ["linux", "macos", "win.exe"]

steps:
- uses: actions/checkout@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- uses: actions/checkout@v2
- uses: actions/checkout@v2
with:
ref: refs/tags/${{ github.event.inputs.bundle-tag }}

It's potentially confusing to build the code from main and upload it as an artifact on a tagged release.

(It would be nice if we could just use the workflow from the release tag too, but workflows can't be dispatched on tags currently.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I've followed Robert's suggestion to make the bundle-tag optional, which now makes this a bit awkward. Do you see a way apart from having two calls to actions/checkout@2 each with an if?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the interests of getting a version of this ready I am going to merge. We can later update this if we see a way to resolve the inconsistency.

@robertbrignull
Copy link
Contributor Author

Thanks for taking on the rest of this @rneatherway.

@rneatherway rneatherway force-pushed the robertbrignull/runner_workflow branch 2 times, most recently from 52e7feb to 59913e8 Compare October 21, 2020 09:04
@rneatherway rneatherway merged commit 6ac5978 into main Oct 21, 2020
@rneatherway rneatherway deleted the robertbrignull/runner_workflow branch October 21, 2020 09:17
This was referenced Oct 26, 2020
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.

4 participants