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

Uncrustify PR Check Update #969

Merged
merged 22 commits into from
Jul 20, 2023
Merged

Uncrustify PR Check Update #969

merged 22 commits into from
Jul 20, 2023

Conversation

Skptak
Copy link
Member

@Skptak Skptak commented Jul 14, 2023

Uncrustify PR Check Update

Description

Change to uncrustify to show files that need to be updated and create a cleaner log on runs

Test Steps

Ran the workflow after the changes to ensure it worked, failed run one, failed run two, and a passing run

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

exit 0
else
echo -e "\033[31;1;43mFormatting check (using Uncrustify) failed...\033[0m"
echo -e "\033[32;3mTo have the code uncrustified for you, please comment '/bot run uncrustify' (without the quotes) on the Pull Request.\033[0m"
Copy link
Member

Choose a reason for hiding this comment

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

I would still like for this to be printed out in case of failure. This repo has a workflow which uncurstifies the code when you comment that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That bot would need to be updated to use the new version of uncrustify. I also dislike that this is the only repo out of all of ours that offers that as an option. I'm going to talk to @A-Zaba about getting this added into the repo and maybe tying it into this one.

Copy link
Member

Choose a reason for hiding this comment

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

That bot would need to be updated to use the new version of uncrustify

Yes.

I also dislike that this is the only repo out of all of ours that offers that as an option

Yes, we should add it to the CI-CD repo and have it in all of our repos.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
A-Zaba
A-Zaba previously approved these changes Jul 20, 2023
else
exit 0
fi
- uses: actions/checkout@v2

Choose a reason for hiding this comment

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

This is probably a nit pick but maybe we want to use actions/checkout@v3? It uses a more up-to-date version of Node.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fully agree with you, but all of our repos are using the checkout@v2 where I would prefer to do that change across all of them at once compared to individual repos.

Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

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

LGTM! Let's ship it :)

@Skptak
Copy link
Member Author

Skptak commented Jul 20, 2023

/bot run uncrustify

1 similar comment
@Skptak
Copy link
Member Author

Skptak commented Jul 20, 2023

/bot run uncrustify

@Skptak
Copy link
Member Author

Skptak commented Jul 20, 2023

/bot run uncrustify

@Skptak
Copy link
Member Author

Skptak commented Jul 20, 2023

/bot run uncrustify

@AniruddhaKanhere
Copy link
Member

/bot run uncrustify

@Skptak Skptak merged commit ecd1307 into FreeRTOS:main Jul 20, 2023
8 checks passed
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.

5 participants