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

Ping deployers if PR is merged with failing lint rules #7026

Closed
Jag96 opened this issue Jan 4, 2022 · 5 comments
Closed

Ping deployers if PR is merged with failing lint rules #7026

Jag96 opened this issue Jan 4, 2022 · 5 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2

Comments

@Jag96
Copy link
Contributor

Jag96 commented Jan 4, 2022

Action Performed:

  1. Merge PR
  2. Deploy PR to staging, PR fails lint

Expected Result:

Confirm that lint passes before attempting staging deploy

Actual Result:

Staging is broken

The issue occurred in #6819 (comment), we should investigate a way to prevent deploying code w/ failing lint to staging/production and ping mobile deployers.

@Jag96 Jag96 added Engineering Weekly KSv2 Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff labels Jan 4, 2022
@Jag96 Jag96 self-assigned this Jan 4, 2022
@AndrewGable
Copy link
Contributor

I have a few questions:

  • Should we run all the PR checks, not just lint?
  • Could we create a deploy blocker issue instead of just pinging a channel? This would hopefully stop a deploy?

@Jag96
Copy link
Contributor Author

Jag96 commented Jan 6, 2022

I like both of those ideas. Creating a deploy blocker issue to prevent deploys seems like a nice improvement. For running additional checks I think e2e might be overkill but adding lint/unit tests/verify podfile seem like good checks to have. I'm thinking these would happen before we create a new version in the preDeploy.yml workflow, but I'll confirm that's the best spot for these.

@Jag96
Copy link
Contributor Author

Jag96 commented Jan 13, 2022

Haven't gotten to test my changes, been dealing with deploy failures all week. Will continue on this once the deploy is fixed.

@Jag96
Copy link
Contributor Author

Jag96 commented Jan 21, 2022

Still haven't gotten to this, posting the diff I was using for testing:

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index aeecd6682..20a1601e9 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -1,6 +1,8 @@
 name: Lint JavaScript

 on:
+  workflow_dispatch:
+    branches-ignore: [staging, production]
   pull_request:
     types: [opened, synchronize]
     branches-ignore: [staging, production]
diff --git a/.github/workflows/preDeploy.yml b/.github/workflows/preDeploy.yml
index 776b14e78..88dfc729d 100644
--- a/.github/workflows/preDeploy.yml
+++ b/.github/workflows/preDeploy.yml
@@ -5,6 +5,28 @@ on:
     branches: [main]

 jobs:
+  confirmPassingBuild:
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Run lint
+        id: lint
+        uses: Expensify/App/.github/actions/triggerWorkflowAndWait@main
+        with:
+          WORKFLOW: lint.yml
+
+      - name: Run tests
+        id: tests
+        uses: Expensify/App/.github/actions/triggerWorkflowAndWait@main
+        with:
+          WORKFLOW: test.yml
+
+      - name: Verify Podfile
+        id: verifyPodfile
+        uses: Expensify/App/.github/actions/triggerWorkflowAndWait@main
+        with:
+          WORKFLOW: verifyPodfile.yml
+
   chooseDeployActions:
     runs-on: ubuntu-latest
     outputs:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index ac8c3d762..53ea28efd 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -1,6 +1,8 @@
 name: Jest Unit Tests

 on:
+  workflow_dispatch:
+    branches-ignore: [staging, production]
   pull_request:
     types: [opened, synchronize]
     branches-ignore: [staging, production]
diff --git a/.github/workflows/verifyPodfile.yml b/.github/workflows/verifyPodfile.yml
index 242cdfe08..eb0cf4db6 100644
--- a/.github/workflows/verifyPodfile.yml
+++ b/.github/workflows/verifyPodfile.yml
@@ -1,6 +1,8 @@
 name: Verify Podfile

 on:
+  workflow_dispatch:
+    branches-ignore: [staging, production]
   pull_request:
     types: [opened, synchronize]
     branches-ignore: [staging, production]

@AndrewGable AndrewGable assigned AndrewGable and unassigned Jag96 Jan 23, 2022
@AndrewGable
Copy link
Contributor

I'll look into this one very soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

4 participants