Ci1 - In progress - test and updates to be done#3299
Conversation
Signed-off-by: Liviu Tomoiaga <Liviu.Tomoiaga@analog.com>
Signed-off-by: Liviu Tomoiaga <Liviu.Tomoiaga@analog.com>
Signed-off-by: Andreea Andrisan <Andreea.Andrisan@analog.com>
|
Some tests are still running, but the implementation and the code can be reviewed |
There was a problem hiding this comment.
Will look more in depth later.
but take a look at
https://github.com/analogdevicesinc/linux-security-vulns/blob/ci/.github/workflows/check-artifact.yml#L70-L78
I use the same ci/download_artifacts.sh to get the artifacts I need and manipulate to my wish -> check cve
my wish could be gen a boot partition at adi-kuiper-gen
| type: string | ||
| version: | ||
| required: false | ||
| default: ${{ github.sha }} |
There was a problem hiding this comment.
this changes the default for /linux from 12 chars to the full 40 chars.
I was considering to default to 40 chars, maybe we can merge and see where it breaks
| @@ -0,0 +1,264 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
this is mostly
https://github.com/analogdevicesinc/linux/blob/ci/ci/download_artifacts.sh
no?
Why not source download_artifacts.sh and just use another 'main' method (if any change required)
There was a problem hiding this comment.
Why I had the feeling that this implementation was only on ci_adapter branch🤔 ? My mistake in this case.
Still, I would suggest to keep the logic apart since the name of the script is download_artifacts.sh and it's not really intuitive from this pov ( just a personal opinion. PS: I think I can come with a better name😅 )
| @@ -0,0 +1,181 @@ | |||
| # SPDX-License-Identifier: GPL-2.0-only | |||
There was a problem hiding this comment.
This is kuiper-linux boot partition logic no? why can't adi-kuiper-gen do this step on their end?
There was a problem hiding this comment.
I do not think so. This script mimics the logic from this one and I'm not sure if it can be added as an end step on adi-kuiper-gen, since the flow needs the artifacts from linux-rpi build. Maybe I'm missing something here, but this is my understanding so far
| @@ -0,0 +1,294 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Also looks like adi-kuiper-gen logic
| cat > "${output_dir}/extlinux.conf" <<EOF | ||
| LABEL Linux Default | ||
| KERNEL ../zImage | ||
| FDT ../${dtb_name} | ||
| APPEND root=/dev/mmcblk0p2 rw rootwait earlyprintk console=ttyS0,115200n8 |
There was a problem hiding this comment.
Also adi-kuiper-gen logic
here we are saying how to boot, beyond dtb_name it is identical to the end of times.
| @@ -0,0 +1,285 @@ | |||
| name: Prepare and Upload to Cloudsmith RPI Artifacts | |||
There was a problem hiding this comment.
Feels duplicated... all because of the kuiper logic...
|
|
||
| jobs: | ||
| prepare_and_upload_sdg_linux_artefacts: | ||
| runs-on: [self-hosted, repo-only] |
There was a problem hiding this comment.
ubuntu-slim is more suitable, only fast internet is needed
| - uses: analogdevicesinc/doctools/checkout@action | ||
|
|
||
| - name: Prepare path | ||
| run: | | ||
| rm -rf dist ; mkdir dist |
There was a problem hiding this comment.
No need to checkout, you don't use anything from the linux git tree at this job
42812d4 to
e8058b2
Compare
|
Hi @liviutomoiaga did you have the opportunity to go through my review? |
|
|
||
| permissions: | ||
| id-token: write | ||
| contents: write |
There was a problem hiding this comment.
| contents: write | |
| contents: read |
|
|
||
| permissions: | ||
| id-token: write | ||
| contents: write |
There was a problem hiding this comment.
| contents: write | |
| contents: read |
| actions: read | ||
|
|
||
| steps: | ||
| - uses: analogdevicesinc/doctools/checkout@action |
There was a problem hiding this comment.
| - uses: analogdevicesinc/doctools/checkout@action |
No need to checkout, you get the files that you need on line 59
| echo "Verifying script content:" | ||
| head -3 ci/prepare_rpi_artifacts.sh | ||
| if [[ ! -s ci/prepare_rpi_artifacts.sh ]] || ! head -1 ci/prepare_rpi_artifacts.sh | grep -q "^#"; then | ||
| echo "ERROR: Script download failed or invalid content" |
There was a problem hiding this comment.
| echo "ERROR: Script download failed or invalid content" | |
| echo "::error ::deploy-rpi-artifacts: Script download failed or invalid content." |
I have been using gh-annotation to surface errors to the summary view of the run
https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands
| @@ -0,0 +1,207 @@ | |||
| name: Deploy SDG Linux Artifacts to Cloudsmith | |||
There was a problem hiding this comment.
see https://github.com/analogdevicesinc/linux/blob/ci-adapter/.github/workflows/expand-to-cloudsmith.yml
why not unify 'xlnx' and 'rpi' in to single .yml and .sh scripts, and 'activate' the differences based on the ${{ github.ref }} value?
We destroying the kernel modules is a mistake on the 'xlnx' path is a mistake, unifying them allows to better compare the difference and mitigate the historical shortcomings.
| artifacts: | ||
| required: true | ||
| type: string | ||
| CLOUDSMITH_REPO: |
There was a problem hiding this comment.
github semantic are lower case, dash separated for inputs, e.g., cloudsmith-repo
| if [[ -n "${{ inputs.PR_NUMBER }}" ]]; then | ||
| export GIT_SHA="${{ github.event.pull_request.head.sha }}" | ||
| else | ||
| export GIT_SHA="${{ github.sha }}" | ||
| fi |
There was a problem hiding this comment.
| if [[ -n "${{ inputs.PR_NUMBER }}" ]]; then | |
| export GIT_SHA="${{ github.event.pull_request.head.sha }}" | |
| else | |
| export GIT_SHA="${{ github.sha }}" | |
| fi | |
| GIT_SHA="${{ github.event.pull_request.head.sha || github.sha }}" |
|
|
||
| # Build version path based on branch type | ||
| if [[ -n "${{ inputs.PR_TARGET_BRANCH }}" ]]; then | ||
| VERSION_PATH="linux/PRs/${{ inputs.PR_TARGET_BRANCH }}/pr_${{ inputs.PR_NUMBER }}" |
There was a problem hiding this comment.
There is mixing inputs with the github.context.
that is problematic
| VERSION_PATH="linux/PRs/${{ inputs.PR_TARGET_BRANCH }}/pr_${{ inputs.PR_NUMBER }}" | |
| pr=${{ github.ref }} # refs/pull/<pr_number>/merge | |
| pr="${ref#refs/pull/}" | |
| pr="${pr%/merge}" | |
| VERSION_PATH="linux/PRs/${{ github.base_ref }}/pr_${pr}" |
Tip: use github search to look for usage examples/inspiration:
https://github.com/search?q=path%3A.github%2Fworkflows+github.base_ref&type=code
There was a problem hiding this comment.
Many thanks for the tip - that's really useful
| MERGE_COMMIT_SHA=$(git rev-parse --short HEAD) | ||
| export GIT_SHA_DATE=$(git show -s --format=%cd --date=format:'%Y-%m-%d %H:%M' ${MERGE_COMMIT_SHA} | sed -e "s/ \|\:/-/g") |
There was a problem hiding this comment.
| MERGE_COMMIT_SHA=$(git rev-parse --short HEAD) | |
| export GIT_SHA_DATE=$(git show -s --format=%cd --date=format:'%Y-%m-%d %H:%M' ${MERGE_COMMIT_SHA} | sed -e "s/ \|\:/-/g") | |
| export GIT_SHA_DATE=$(git show -s --format=%cd --date=format:'%Y-%m-%d %H:%M' ${GIT_SHA} | sed -e "s/ \|\:/-/g") |
In the previous line you said # Get git SHA (PR source commit or build commit) but then now you are considering only the MERGE_COMMIT_SHA case, but actually looking at head.
| export GIT_SHA_DATE=$(git show -s --format=%cd --date=format:'%Y-%m-%d %H:%M' ${MERGE_COMMIT_SHA} | sed -e "s/ \|\:/-/g") | ||
|
|
||
| # Determine branch name (works for main, release/*, feature/*, etc.) | ||
| BRANCH_NAME=$(echo "${{ inputs.BUILD_SOURCEBRANCH }}" | awk -F'/' '{print $NF}') |
| default: 'sdg-linux' | ||
| BUILD_SOURCEBRANCH: | ||
| required: false | ||
| default: ${{ github.ref_name }} |
There was a problem hiding this comment.
${{ github.ref_name }}: For pull requests that were not merged, the format is <pr_number>/merge.
https://docs.github.com/en/actions/reference/workflows-and-actions/variables
|
Is this supposed for pull_request|push events, or workflow_dispatch too? This defines if you need the |
|
|
||
|
|
||
| jobs: | ||
| prepare_and_upload_sdg_linux_artefacts: |
|
|
||
| jobs: | ||
| prepare_and_upload_sdg_linux_artefacts: | ||
| runs-on: [self-hosted, repo-only] |
There was a problem hiding this comment.
can't we do cloud-hosted? Afaik you should be able to upload to cloudsmith from the cloud runners as well
| - name: Get sources | ||
| run: | | ||
| file=$(echo "${{ github.workflow_ref }}" | cut -d'/' -f3- | cut -d'@' -f1) | ||
| workflow_ref=$(awk ' |
There was a problem hiding this comment.
some comments would be nice
| get_file ci/prepare_sdg_linux_artifacts.sh | ||
| get_file ci/lib.sh | ||
| get_file ci/prepare_artifacts_structure.sh | ||
|
|
||
| chmod +x ci/*.sh |
There was a problem hiding this comment.
ideally, these files should be committed as executable (git update-index --chmod=+x script.sh)
| - name: Download workflow artifacts | ||
| run: | | ||
| gh-get-workflow-artifacts() | ||
| { |
There was a problem hiding this comment.
you should keep the same style across the script, curly bracket should be on L82
| # Configuration | ||
| SOURCE_DIRECTORY="${SOURCE_DIRECTORY:-$(pwd)}" | ||
| TIMESTAMP="${TIMESTAMP:-$(date +%Y_%m_%d-%H_%M_%S)}" | ||
| BUILD_SOURCEBRANCHNAME="${BUILD_SOURCEBRANCHNAME:-main}" |
There was a problem hiding this comment.
maybe let's not use default for branch names
| echo $workflow_ref | ||
|
|
||
| org_repo="$(echo "$workflow_ref" | cut -d'/' -f1-2)" | ||
| ref="$(echo "$workflow_ref" | cut -d'@' -f2)" |
There was a problem hiding this comment.
this is a bit of overengineering for things that will mostly stay static, e.g. org_repo would always be analogdevicesinc, right? And the ref should always be ci?
| fi | ||
|
|
||
| source ci/lib.sh | ||
| source ci/prepare_artifacts_structure.sh |
There was a problem hiding this comment.
test if these sources are really needed (you already sourced them on L147)
| "$tags" \ | ||
| "$version" || exit 1 | ||
| fi | ||
|
|
| ["arria10"]="socfpga_adi_defconfig-gcc-arm/boot/zImage" | ||
| ["cyclone5"]="socfpga_adi_defconfig-gcc-arm/boot/zImage" | ||
| ["zynq"]="zynq_xcomm_adv7511_defconfig-gcc-arm/boot/uImage" | ||
| ["versal"]="adi_versal_defconfig-gcc-arm64/boot/Image" | ||
| ["zynqmp"]="adi_zynqmp_defconfig-gcc-arm64/boot/Image" |
There was a problem hiding this comment.
Maybe these can be replaced with regexes?
Signed-off-by: Liviu Tomoiaga <Liviu.Tomoiaga@analog.com>
I was thinking to have the the following code block implemented in the |
Hi @gastmaier I'm looking through it. There are some nice suggestions from your side and from @bia1708 that I would like to test. I'll set this PR to Draft for the moment. |
PR Description
This PR adds CI workflows and scripts for preparing and deploying Linux build artifacts to Cloudsmith:
archives, and upload to Cloudsmith
kernel images, DTBs, and git metadata
PR Type
PR Checklist