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: fix main workflow #72

Merged
merged 2 commits into from
Mar 4, 2024
Merged

ci: fix main workflow #72

merged 2 commits into from
Mar 4, 2024

Conversation

nealrichardson
Copy link
Collaborator

#60 updated the pull request workflow but also needed to update the main workflow, which was not triggered on the PR. This PR fixes that and modifies that workflow to run when the workflow file is touched, which should trigger it on this PR.

That said, I'm not sure why we need a separate workflow to run on main from the one we run on PRs.

@nealrichardson nealrichardson requested a review from tdstein March 4, 2024 19:44
Copy link

github-actions bot commented Mar 4, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
282 246 87% 80% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 96cf05a by action🐍

Comment on lines 6 to 8
pull_request:
paths:
- '.github/workflows/cpp.yml'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to commit this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦 Yes I did, but I meant to edit it to be main.yaml instead, which would explain why it didn't run here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
pull_request:
paths:
- '.github/workflows/cpp.yml'
pull_request:
paths:
- '.github/workflows/main.yaml'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh neat! So, to confirm my understanding... this runs main.yaml anytime the file is changed on a pull request?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct--you can see that it just ran now that I fixed it. So it still wouldn't have caught the change in #60 since I didn't touch this file. But if we ever edit this file in the future, the PR will run it, we won't have to wait for it to land on main.

Since this is basically testing the Makefile, we could also have it run when the Makefile is modified?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. I like the idea of workflows being task-focused instead of branch-focused. e.g., makefile.yaml instead of main.yaml or something...

@tdstein
Copy link
Collaborator

tdstein commented Mar 4, 2024

That said, I'm not sure why we need a separate workflow to run on main from the one we run on PRs.

Out of habit, I set up main to verify that the default development workflow (e.g., make && make install) is functional. This is just the habit I've created for myself. Open to other ideas, though!

The only current difference is that the pull request performs the coverage report.

@nealrichardson
Copy link
Collaborator Author

That said, I'm not sure why we need a separate workflow to run on main from the one we run on PRs.

Out of habit, I set up main to verify that the default development workflow (e.g., make && make install) is functional. This is just the habit I've created for myself. Open to other ideas, though!

The only current difference is that the pull request performs the coverage report.

Got it.

I would think we'd want coverage running on main so that we could track it over time. That said, I don't plan on spending much time thinking about our coverage numbers.

@nealrichardson nealrichardson merged commit f699c06 into main Mar 4, 2024
13 checks passed
@nealrichardson nealrichardson deleted the fix-ci branch March 4, 2024 21:06
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.

2 participants