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

CI: Add support for GitHub Actions #6885

Merged
merged 2 commits into from
Jan 16, 2021
Merged

CI: Add support for GitHub Actions #6885

merged 2 commits into from
Jan 16, 2021

Conversation

alessandrogario
Copy link
Member

@alessandrogario alessandrogario commented Jan 6, 2021

The original intent was to just add Linux support, but I ended up adding all platforms (sadly, I wasn't able to keep one file per OS because jobs can only depend on other jobs defined on the same files).

I think it looks decent for an initial version, as it's at least in feature parity compared to Azure; I would like to start looking into the signing options with @directionless's guidance once this is merged

The workflow should always attach the following artifacts:

  1. Windows packages (nupkg, msi)
  2. Linux packages (deb, ddeb, release rpm, debuginfo rpm, tar.gz)
  3. macOS packages (pkg, tar.gz)
  4. cppcheck logs (release and debug, Linux only)

An additional difference compared to Azure is that no build jobs are started if the code fails the format check

@alessandrogario alessandrogario added Linux CI/CD Anything about the Continuous Integration or Continuous Deployment tool used by the repository labels Jan 6, 2021
@directionless
Copy link
Member

when i was playing, i was trying to split. I left it at https://github.com/directionless/osquery/blob/master/.github/workflows/macos.yaml

@alessandrogario alessandrogario changed the title CI: Add a Linux job for GitHub Actions CI: Add support for GitHub Actions Jan 8, 2021
Copy link
Member

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I like this.

I don't know if we can converge the different os jobs in the workflow. It feels a bit weird to have so many duplicated steps. But, I'd accept it.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@directionless
Copy link
Member

directionless commented Jan 8, 2021

From this, it looks like the github stuff is notable slower than the azure stuff. I guess we'll see if that's an issue.

@Smjert
Copy link
Member

Smjert commented Jan 8, 2021

From this, it looks like the github stuff is notable slower than the azure stuff. I guess we'll see if that's an issue.

I think there's something wrong going with ccache; the agent instances should be very similar if not identical to Azure Pipelines.
I'm saying this because without a ccache properly available, on Azure Pipelines too it would take > 1h to build Linux.
Also, the max job timeout is 6h or so, there's a property we have to set on the job to increase such timeout which by default is at 1h.

@alessandrogario alessandrogario marked this pull request as ready for review January 13, 2021 19:45
Copy link
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

What if the individual platform builds don't depend on the check code style step? Then we might be able to split this up into separate files and also the run could complete quicker due to further parallelization.

@alessandrogario
Copy link
Member Author

What if the individual platform builds don't depend on the check code style step? Then we might be able to split this up into separate files and also the run could complete quicker due to further parallelization.

The check_code_style job was implemented as a blocking prerequisite to make sure that an invalid PR (which could receive a burst of additional commits that attempts to fix formatting) will not cause valid PRs to starve for workers

Copy link
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

I wonder if it's possible to have a new push to a branch cancel the runs of the existing jobs. That could resolve the starvation issue. Happy to go ahead with this.

@Smjert
Copy link
Member

Smjert commented Jan 15, 2021

I wonder if it's possible to have a new push to a branch cancel the runs of the existing jobs. That could resolve the starvation issue. Happy to go ahead with this.

That is what it happens already, you won't have more than a run worth of jobs occupied, but I guess what we're trying to avoid is to still occupy those jobs if the formatting is going to fail again and will require another change.
It often depends how soon the PR author notices that the build failed for formatting issues.

@alessandrogario alessandrogario merged commit 2729225 into osquery:master Jan 16, 2021
@alessandrogario alessandrogario deleted the alessandro/ci/add-linux-github-actions-job branch January 16, 2021 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Anything about the Continuous Integration or Continuous Deployment tool used by the repository Linux macOS Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants