Skip to content

Conversation

@chtruong814
Copy link
Contributor

@chtruong814 chtruong814 commented May 27, 2025

Enable initial unit tests and remove auto-formatting in PRs

  • Add unit test placeholder
  • Update copy-pr-bot to require /ok to test to run the CI job
  • Remove the auto-formatting in favor of just checking it at linting

Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
- maanug-nv
auto_sync_draft: false
auto_sync_ready: true
auto_sync_ready: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I removed the Run CICD label need for now and I thought we were thinking better to use /ok to test? Or do you think we should just let any commit start running the CI? Open to either tbh. Early enough that we can wait to see if it's a pain without it.

Comment on lines 192 to +197
NUM_CANCELLED=$(gh run view $RUN_ID --json jobs -q '[.jobs[] | select(.conclusion == "cancelled") | .name] | length')

if [[ $NUM_FAILED -eq 0 && $NUM_CANCELLED -eq 0 && ("$HAS_LABEL" == "true" || "$IS_SCHEDULED" == "true") ]]; then
if [[ $NUM_FAILED -eq 0 && $NUM_CANCELLED -eq 0 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a headsup this might not work currently. IIRC, gh cli doesn't have access to a repo with the anonymous github.token token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would just silently fail? We have the PAT in the repo as a secret. Maybe we just use that?

ko3n1g
ko3n1g previously approved these changes May 27, 2025
Copy link
Contributor

@ko3n1g ko3n1g left a comment

Choose a reason for hiding this comment

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

LGTM. Easy review since this is just one of the early onboarding PRs. However, one question and one remark.

@chtruong814 chtruong814 merged commit b4cd7b4 into main May 27, 2025
10 of 11 checks passed
@chtruong814 chtruong814 deleted the chtruong/ci-tests branch May 27, 2025 22:29
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.

3 participants