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

CI Improvements - Part 1 #1821

Merged
merged 7 commits into from
Jul 17, 2024
Merged

Conversation

SergioGasquez
Copy link
Member

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

  • HIL workflow: Fix the condition to erase flash when tests fail. Source
  • Remove path-ignore to avoid CI not being triggered.
  • Build documentation

Testing

  • The HIL workflow cant be tested easily, we need to monitor the workflow and verify that upon a test failure, the erase-flash step is executed.
  • Documentation was built in my fork https://github.com/SergioGasquez/esp-hal/actions/runs/9972410348/attempts/1. I ran the workflow twice, both took ~10min45. Currently, CI takes between 10 to 14 mins, so I guess we didn't increased it too much.
    • Happy to move this to a nightly CI workflow, but it looks like it doesnt add much time.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - I don't really understand the fix for the flash-erase but that's most probably a lack of knowledge about GitHub workflows on my side

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SergioGasquez
Copy link
Member Author

LGTM - I don't really understand the fix for the flash-erase but that's most probably a lack of knowledge about GitHub workflows on my side

I don't fully understand it either, but it wasn't working before and the GH docs has an example using that same condition

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@SergioGasquez SergioGasquez added the skip-changelog No changelog modification needed label Jul 17, 2024
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks, this is a nice improvement!

@MabezDev MabezDev added this pull request to the merge queue Jul 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 17, 2024
@SergioGasquez SergioGasquez added this pull request to the merge queue Jul 17, 2024
Merged via the queue into esp-rs:main with commit 32be53d Jul 17, 2024
31 checks passed
@SergioGasquez SergioGasquez deleted the feat/improve-ci branch July 17, 2024 16:09
@AnthonyGrondin
Copy link
Contributor

For the flash-erase fix, it seems to be because Github's implementation require failure() to be present in the if statement, else it automatically uses success()

failure with conditions 🔗
You can include extra conditions for a step to run after a failure, but you must still include failure() to override the default status check of success() that is automatically applied to if conditions that don't contain a status check function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
5 participants