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

🚧Improve build and status check workflows #743

Closed
9 tasks done
CalvinWilkinson opened this issue Sep 21, 2023 · 0 comments · Fixed by #746
Closed
9 tasks done

🚧Improve build and status check workflows #743

CalvinWilkinson opened this issue Sep 21, 2023 · 0 comments · Fixed by #746
Assignees
Labels
cicd CI/CD and workflow related work only high-priority High Priority preview Done while in preview

Comments

@CalvinWilkinson
Copy link
Member

CalvinWilkinson commented Sep 21, 2023

Complete The Item Below

  • I have updated the title without removing the 🚧 emoji.

Description

Improve build and status check workflows for fork pull requests.

This is done by improving the workflows to be executed in a manner that is still secure but also checks out the fork repo's head branch so the build and tests run on the new code that the pull request provides.

Currently, the workflows are using the 'pull_request' trigger. This runs the build and tests for a PR on the merge commit, which is ideal to ensure that the new code does not break the build and tests with it being mixed with the original code from the base branch.

The problems with this is:

  1. The workflow uses org and repo vars. The trigger 'pull_request' prevents access to these variables to empty values are used and these are required for the workflow to operate correctly.
  2. They will not auto-run for the outside contributor because they have to be approved
  3. There is a security risk because if they introduced code changes to workflows on the head branch, and it is approved, that could introduce malicious workflow code.

Solution:
Use the workflow trigger 'pull_request_target' for a trigger but purposefully check out the head branch of the fork. This will make sure that the workflow used by GitHub will not be the workflow changes that might come included with the pull request. Instead, it uses the workflows from the source repo and base branch which of course are not changed.

This approach gives us more security but still builds and tests against the new code.

The downside to this is that builds and tests will not be run against the head and base branch code mixed together. There is no way around this when it comes to outside contributors while staying secure.

Checkout action:

- name: Checkout code
  uses: actions/checkout@v2
  with:
    ref: ${{ github.event.pull_request.head.ref }}
    repository: ${{ github.event.pull_request.head.repo.full_name }}

To implement this, we are going to need to update the Infrastructure reusable workflows named build-csharp-project.yml and run-csharp-tests.yml to take in 3 more workflow inputs. The checkout of the repo is performed in the reusuable workflows, not in the velaptor ones.

Change the workflow target to 'pull_request_target'.

Acceptance Criteria

  • Build status check workflow target changed to 'pull_request_target'
  • Build status check checkout action updated
  • Unit test status check workflow target changed to 'pull_request_target'
  • Unit test status check checkout action updated

ToDo Items

  • Change type labels added to this issue. Refer to the Change Type Labels section below.
  • Priority label added to this issue. Refer to the Priority Type Labels section below.
  • Issue linked to the correct milestone (if applicable).

Issue Dependencies

Related Work

No response

Additional Information:

Change Type Labels

Change Type Label
Bug Fixes 🐛bug
Breaking Changes 🧨breaking changes
New Feature ✨new feature
CICD Changes ♻️cicd
Config Changes ⚙️config
Performance Improvements 🏎️performance
Code Doc Changes 🗒️documentation/code
Product Doc Changes 📝documentation/product

Priority Type Labels

Priority Type Label
Low Priority low priority
Medium Priority medium priority
High Priority high priority

Code of Conduct

  • I agree to follow this project's Code of Conduct.
@CalvinWilkinson CalvinWilkinson added cicd CI/CD and workflow related work only high-priority High Priority preview Done while in preview labels Sep 21, 2023
@CalvinWilkinson CalvinWilkinson self-assigned this Sep 21, 2023
@CalvinWilkinson CalvinWilkinson added this to the v1.0.0-preview.28 milestone Sep 21, 2023
CalvinWilkinson added a commit that referenced this issue Sep 23, 2023
CalvinWilkinson added a commit that referenced this issue Sep 25, 2023
* Start work for issue #743

* ci: update build status check to be more secure

* ci: update test status check to be more secure

* ci: prevent workflow from running on renovate branches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cicd CI/CD and workflow related work only high-priority High Priority preview Done while in preview
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant