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

Fix #798, Update build/test workflows to include sample/lab apps #799

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thnkslprpt
Copy link
Contributor

Checklist

Describe the contribution

Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.).
I've tested the build-run-app workflow with sample_app, sch_lab and sample_lib, and the unit-test-coverage workflow with sample_app and sample_lib - all working as expected.

Expected behavior changes
This PR will allow immediate notification of a regression or issue with the build/tests in the sample/lab repositories when pushing a branch or creating a PR.

System(s) tested on
Debian 12 using the current main branch of cFS bundle.

Contributor Info
Avi Weiss   @thnkslprpt

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments in the code. I like the concept but we never documented that the words "sample" or "lab" will never appear in app names outside of the framework apps.

I'd like to see if this can more directly test if the app name is already in the respective lists, or maybe instead of patching the sample, could it generate the targets.cmake and/or cfe_es_startup.scr with the content specifically for that app?

run: |
echo "APP_UPPER=$(echo ${{ inputs.app-name }} | sed 's/[a-z]/\U&/g')" >> $GITHUB_ENV
echo "APP_LOWER=$(echo ${{ inputs.app-name }} | sed 's/[A-Z]/\L&/g')" >> $GITHUB_ENV
echo "IS_SAMPLE_OR_LAB=$(echo ${{ inputs.app-name }} | grep -iqE 'sample|lab' && echo true || echo false)" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only (slight) concern is with this logic. I can see why you did this -- that the "sample" and "lab" apps are already in the sample config, thus we need to not add them again or else we'd end up with a duplicate entry.

My concern is that this will break if someone changes the sample config such that this assumption is not true anymore. I also don't want to assume that there will never be another app with either "sample" or "lab" in its name.

Could we do something more direct -- that is, instead of checking the app name, check if the script already has the entry? That is, if running on "to_lab" -- just check if the cfe_es_startup.scr already has an entry for to_lab, and skip the update if it does?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least -- another idea is could this be a parameter that is passed into the workflow? Something like "is_framework_app" ... with a default value of false. Then when TO/CI/SCH Lab and Sample App invoke this workflow, they can pass it as true, but everything else remains as it was. This would accomplish the same goal without making this assumption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for sample/lab apps and library to build-run and unit-test workflows
2 participants