analogdevicesinc / precision-converters-firmware

Precision Converters Embedded Firmware Repository
https://analogdevicesinc.github.io/precision-converters-firmware/
Apache License 2.0
15 stars 17 forks source link

workflows: Update GitHub based workflows pipeline #59

Closed SaikiranGudla closed 7 months ago

SaikiranGudla commented 7 months ago

Add two top-level pipelines main and pr which will invoke the workflows based on the action.

Make the build-projects, check-versions, lint workflows reusable and call them from main and pr pipelines.

SaikiranGudla commented 7 months ago

Forked the repo to test the updated pipeline design: https://github.com/SaikiranGudla/precision-converters-firmware/actions/runs/8513283509

Will add doc-build, deploy-doc workflows to the sphinix-doc branch. That will also take care of the uploading and downloading of sphinix, doxygen html build artifacts.

SaikiranGudla commented 7 months ago

Forked the repo to test the updated pipeline design: https://github.com/SaikiranGudla/precision-converters-firmware/actions/runs/8513283509

Will add doc-build, deploy-doc workflows to the sphinix-doc branch. That will also take care of the uploading and downloading of sphinix, doxygen html build artifacts.

image

ribdp commented 7 months ago

A couple of questions here just so I understand the changes -

Why do we want to make the workflows reusable? Was there anything lacking in the original approach where we had those as separate build steps in a single .yaml file? Right now there's separate main.yaml and pr.yaml. The only difference between them (that I see) seems to be the presence (or absence) of the yet-to-be-added deploy-doc. Can we not add that as a separate build step and just add a git ref check there? I guess that would accomplish the same result?

Let me know if have misunderstood or missed anything.

gastmaier commented 7 months ago

Hi @ribdp this approach is more maintainable, since you get an overview of the workflows per context (push to main/pr) and provides simpler granular control over the pipeline. For example, an improvement that could already be done is have "lint" and "check-versions" run in parallel by changing

  lint:
    uses: ./.github/workflows/lint.yaml
    needs: [check-versions]

to

  lint:
    uses: ./.github/workflows/lint.yaml
    needs: [build-projects]

and a future tests.yaml workflow could also be run in parallel.

add a git ref check there

Sure that's an option, but on: pull_request: sure is simpler and avoids all the shenanigans that the git cli might return (error codes, warning strings, etc).

What could be done is have a single main.yml with the if condition and both push: main and pull_request:

on:
  push:
    branches:
      - main
  pull_request:

jobs:
  (...)
  deploy-doc:
    if: github.ref == 'refs/branch/main'
    uses: ./.github/workflows/build-doc.yml
    needs: [build-doc]
    secrets: inherit

and remove pr.yml (considering the workflows on #60) refs: https://docs.github.com/en/actions/using-jobs/using-conditions-to-control-job-execution https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability

SaikiranGudla commented 7 months ago

Added one more implementation on similar grounds here: https://github.com/SaikiranGudla/precision-converters-firmware/tree/workflow_imp_2/.github/workflows

We'll now have two workflows one each for build-projects, documentation. The jobs check-versions, lint which are part of the build-projects run in parallel now and build-projects needs both of them.

Documentation workflow (doc-build job) also runs in parallel and deploy-doc job runs in the case of push to the main with the github ref check once the doc-build job gets complete.

image

image

image

@ribdp, this approach seems better to me. Let me know your thoughts on this so that I can proceed with the changes.

ribdp commented 7 months ago

Hi @ribdp this approach is more maintainable, since you get an overview of the workflows per context (push to main/pr) and provides simpler granular control over the pipeline. For example, an improvement that could already be done is have "lint" and "check-versions" run in parallel by changing

  lint:
    uses: ./.github/workflows/lint.yaml
    needs: [check-versions]

to

  lint:
    uses: ./.github/workflows/lint.yaml
    needs: [build-projects]

and a future tests.yaml workflow could also be run in parallel.

add a git ref check there

Sure that's an option, but on: pull_request: sure is simpler and avoids all the shenanigans that the git cli might return (error codes, warning strings, etc).

What could be done is have a single main.yml with the if condition and both push: main and pull_request:

on:
  push:
    branches:
      - main
  pull_request:

jobs:
  (...)
  deploy-doc:
    if: github.ref == 'refs/branch/main'
    uses: ./.github/workflows/build-doc.yml
    needs: [build-doc]
    secrets: inherit

and remove pr.yml (considering the workflows on #60) refs: https://docs.github.com/en/actions/using-jobs/using-conditions-to-control-job-execution https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability

Hi @gastmaier , thanks for the detailed response! Agree with the suggestion and it makes sense to me (@SaikiranGudla and I also discussed this offline earlier today and converged on a similar approach).

ribdp commented 7 months ago

Added one more implementation on similar grounds here: https://github.com/SaikiranGudla/precision-converters-firmware/tree/workflow_imp_2/.github/workflows

We'll now have two workflows one each for build-projects, documentation. The jobs check-versions, lint which are part of the build-projects run in parallel now and build-projects needs both of them.

Documentation workflow (doc-build job) also runs in parallel and deploy-doc job runs in the case of push to the main with the github ref check once the doc-build job gets complete.

image

image

image

@ribdp, this approach seems better to me. Let me know your thoughts on this so that I can proceed with the changes.

This looks good to me as well

SaikiranGudla commented 7 months ago

Change Log: Implemented the method as discussed in the comments. The actions tab reflects the same.

SaikiranGudla commented 7 months ago

Changelog:

Corrected incorrect branch reference