Documenting and cleaning up our github action workflows part 1 #6153

aryx · 2022-09-21T16:11:55Z

test plan:
wait for CI to kick in and check if everything is green

PR checklist:

Purpose of the code is evident to future readers
Tests included or PR comment includes a reproducible test plan
Documentation is up-to-date
A changelog entry was added to changelog.d for any user-facing change
Change has no security implications (otherwise, ping security team)

If you're unsure on any of this, please see:

test plan: wait for CI to kick in and check if everything is green

aryx · 2022-09-21T20:18:13Z

To whoever review, the goal is to better document our workflow. I've put some TODOs in the yaml file when I don't know something so if you have the knowledge it would be great if in your review you comment next to it so then I can fill those TODOs with what you've explained.

spencerdrak · 2022-09-21T20:22:36Z

.github/workflows/e2e-semgrep-ci.yml

@@ -1,4 +1,7 @@
+#TODO: The goal of this workflow is ???


This workflow is set up to do some very basic E2E checks - makes sure that semgrep will still run both on a pull request and via semgrep ci. The context here is that we had some cases where releases went out that were broken in simple ways, and this gives us some confident that nightly semgrep still works.

It also allows us to confirm fail-open behavior is still functioning as expected (e.g., we aren't passing on blocking findings)

spencerdrak · 2022-09-21T20:25:46Z

.github/workflows/README.md

+  release.yml has been accepted or is hanging around unmerged
+
+- homebrew-core-head.yml: cron to check that the Homebrew Core Formula
+  created by release.yml works (TODO: why can't this be part of validate-release.yml?


These jobs take >60 minutes to run, and require that the latest version of the homebrew formula is up to date, which only occurs after release. The PRs to homebrew also are merged when the homebrew teams gets to them - we shouldn't block on actions we can't control.

github-actions · 2022-09-21T20:28:40Z

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

Review the changes at bc7959c

Accept the new snapshots with

git fetch origin && git cherry-pick bc7959c1e9a2cc7d054614352bae1aef3b8fd366 && git push

spencerdrak · 2022-09-21T20:26:08Z

.github/workflows/README.md

+  created by release.yml works (TODO: why can't this be part of validate-release.yml?
+  because it usually take some time for homebrew developers to accept the PRs
+  so we can't test directly? hence find-old-brew-prs.yml and homebrew-core-head.yml
+  workflows?)


Correct assumption.

spencerdrak · 2022-09-21T20:26:49Z

.github/workflows/lint.yml

 name: lint
+


I haven't touched much of this workflow, will try and answer questions but may leave gaps based on context.

@brendongo might have context.

spencerdrak · 2022-09-21T20:27:40Z

.github/workflows/lint.yml

  pre-commit:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
+        #TODO: why do we need this ref:? doesn't checkout@v3 grabs the right branch by default?


It should. I can't speak for this specific case, but in the workflows I've been a part of implementing, we've set this explicitly to remove assumptions and make it clear for follow on maintainers.

spencerdrak · 2022-09-21T20:28:04Z

.github/workflows/lint.yml

        with:
          GET_FILE_DIFF: true
          PATTERNS: cli/src/semgrep/**
      - id: changelog_diff
        name: Get changelog diff
        uses: technote-space/get-diff-action@v6.1.0
+        #TODO: should we get rid of this whole job now that we use changelog.d/?


Probably, yes.

spencerdrak · 2022-09-21T20:28:54Z

.github/workflows/tests.yml

@@ -17,6 +21,7 @@ jobs:
    runs-on: ubuntu-latest
    container: returntocorp/ocaml:alpine-2022-06-09
    steps:
+      #TODO: why do we need this? What does not work if we don't have this?


This was needed the last time I looked at this part of the workflow (~3 months ago). Maybe @underyx knows.

I think @mjambon actually wrote those pre-checkout fixes

spencerdrak · 2022-09-21T20:29:45Z

.github/workflows/e2e-semgrep-ci.yml

@@ -1,4 +1,7 @@
+#TODO: The goal of this workflow is ???


It also allows us to confirm fail-open behavior is still functioning as expected (e.g., we aren't passing on blocking findings)

spencerdrak · 2022-09-21T20:31:27Z

.github/workflows/homebrew-core-head.yml

@@ -1,9 +1,12 @@
-# Verify Homebrew Core Formula Works
+# Cron to verify that the Homebrew Core Formula Works.
+# This formula is created by ???


This formula is created by our release process with the PR to homebrew/homebrew-core. What this workflow does is uses the latest version of the formula at that repo, but develop branch source code from our PR. This serves two purposes:

Verifies that our changes don't break brew

Gives us time before release to fix these issues and adjust our homebrew formula if needed.

spencerdrak · 2022-09-21T20:32:42Z

.github/workflows/start-release.yml

@@ -45,6 +52,7 @@ jobs:
          submodules: "recursive"
          ref: "${{ github.event.repository.default_branch }}"
          token: "${{ steps.token.outputs.token }}"
+      #TODO: Why do we need this? checkout@v3 does not get the tags by default?


See comment below, however, no. checkout@v3 only gets tags if you do a fully checkout, which is too heavyweight. We don't want all branches and everything that ever existed on the repo, so we just do a lightweight checkout and then get the tags ourselves.

spencerdrak · 2022-09-21T20:33:19Z

.github/workflows/README.md

+- revert-semgrep-docker-image.yml: interactive workflow
+  to manually revert the semgrep 'latest' docker image.
+
+- e2e-semgrep-ci.yml: ???


See the comment on the workflow file for a description.

github-actions · 2022-09-21T20:57:51Z

📸 The pytest shapshots changed in your PR.
Please carefully review these changes and make sure they are intended:

Review the changes at f83437c

Accept the new snapshots with

git fetch origin && git cherry-pick f83437c65a8022812e24b1f0f7cf5151bdfe03cf && git push

brendongo · 2022-09-21T21:11:38Z

.github/workflows/lint.yml

      - name: Fetch semgrep-cli submodules
        run: git submodule update --init --recursive --recommend-shallow cli/src/semgrep/lang cli/src/semgrep/semgrep_interfaces
+      #TODO: why this is needed? pre-commit/action@v3 requires this setup-python to run?
+      # what is this cache-dependency-path?


This caches the pip installed dependencies that pre-commit uses. Otherwise it will need to reinstall everytime

And yes pre-commit is a python script :D https://github.com/pre-commit/pre-commit

so each time you use the pre-commit/action, you also need to setup python before? The github action don't have a way to pull automatic dependencies?

brendongo · 2022-09-21T21:14:06Z

.github/workflows/lint.yml

      - uses: actions/setup-python@v4
        with:
          python-version: "3.10"
          cache: pip
          cache-dependency-path: .github/workflows/lint.yml
      - uses: pre-commit/action@v3.0.0

+  #TODO: what is the difference with the 'pre-commit' job above? looks only
+  # the extra_args below but what is it for? What will not work without that?


The extra args runs https://github.com/returntocorp/semgrep/blob/develop/.pre-commit-config.yaml#L150
which dont run on normal pre-commit since they would slow down pre-commit on local development. The intention is a test that runs semgrep-pre-commit. So we can also split this out to a different config?

why not add just the

with: extra_args: --hook-stage manual

in the pre-commit job instead of duplicating all the parts before and make it a separate job?
Because it can run in parallel then?

I don't see any difference though oin the pre-commit config file between:

# Dogfood! running semgrep in pre-commit! - repo: https://github.com/returntocorp/semgrep rev: "v0.100.0" hooks: - id: semgrep name: Semgrep Python types: [python] exclude: "^cli/tests/.+$|^scripts/.+$|^cli/setup.py$" args: ["--config", "https://semgrep.dev/p/python", "--error"] - id: semgrep name: Semgrep Bandit types: [python] exclude: "^cli/tests/.+$|^scripts/.+$|^cli/setup.py$" args: ["--config", "https://semgrep.dev/p/bandit", "--error"]

and

# Run develop semgrep. Only used in CI since it's slower for local developmemt. # To run locally use `pre-commit run --hook-stage manual semgrep-docker-develop` - repo: https://github.com/returntocorp/semgrep rev: "v0.100.0" hooks: - id: semgrep-docker-develop name: Semgrep Develop Python types: [python] exclude: "^cli/tests/.+$|^scripts/.+$|^cli/setup.py$" args: ["--config", "p/python", "--error"] stages: [manual] - id: semgrep-docker-develop name: Semgrep Develop Bandit types: [python] exclude: "^cli/tests/.+$|^scripts/.+$|^cli/setup.py$" args: ["--config", "p/bandit", "--error"] stages: [manual]

in both cases we use rev: "v0.100.0" so we don't really test the develop docker image here.

* Documenting and cleaning up our github action workflows test plan: wait for CI to kick in and check if everything is green * more comments * more * adding the comments of brendon and spencer (and adding a few TODOs)

Documenting and cleaning up our github action workflows

62a04a4

test plan: wait for CI to kick in and check if everything is green

aryx requested a review from a team September 21, 2022 16:11

nmote approved these changes Sep 21, 2022

View reviewed changes

more comments

3bbbecf

aryx requested review from spencerdrak, underyx, brendongo, emjin and mjambon September 21, 2022 20:16

aryx changed the title ~~Documenting and cleaning up our github action workflows~~ Documenting and cleaning up our github action workflows part 1 Sep 21, 2022

aryx requested a review from nmote September 21, 2022 20:19

spencerdrak reviewed Sep 21, 2022

View reviewed changes

more

fd1579c

brendongo reviewed Sep 21, 2022

View reviewed changes

aryx added 2 commits September 22, 2022 10:22

Merge branch 'develop' into cleanup_workflows

3894395

adding the comments of brendon and spencer (and adding a few TODOs)

7913304

aryx merged commit 35380bb into develop Sep 22, 2022

aryx deleted the cleanup_workflows branch September 22, 2022 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documenting and cleaning up our github action workflows part 1 #6153

Documenting and cleaning up our github action workflows part 1 #6153

aryx commented Sep 21, 2022 •

edited

Loading

aryx commented Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

github-actions bot commented Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

aryx Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

spencerdrak Sep 21, 2022

github-actions bot commented Sep 21, 2022

brendongo Sep 21, 2022

brendongo Sep 21, 2022

aryx Sep 22, 2022

brendongo Sep 21, 2022

aryx Sep 22, 2022

aryx Sep 22, 2022

Documenting and cleaning up our github action workflows part 1 #6153

Documenting and cleaning up our github action workflows part 1 #6153

Conversation

aryx commented Sep 21, 2022 • edited Loading

aryx commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryx commented Sep 21, 2022 •

edited

Loading