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

Update run conditions for workflows #1494

Merged
merged 13 commits into from Aug 6, 2023
Merged

Conversation

bquan0
Copy link
Contributor

@bquan0 bquan0 commented Jul 25, 2023

Description

Makes build_test.yml run when it or the composite action it uses is updated AND it is a push to the develop branch in the main repo (not forks). Also makes install_script.yml run on every push to develop and every pull_request to develop.

Motivation and Context

Closes #1292 and #1491, which request the two features I mentioned above.

Behavior

The previous behavior was that build_test.yml ran even though there were no changes to itself or the dockerfile that built the image it tested. This is because it used paths-ignore: to filter out files instead of using paths: to specify files to check if they have been changed.
The previous behavior of install_script.yml was that it only ran when it or the install script ubuntu.sh was changed. The current behavior is that it now also runs whenever there is a push or pull to develop.

Other Information

One thing I was wondering was if I should add docker/* to the paths: of build_test.yml since it's also in docker_publish.yml. I didn't add it because I thought it would be redundant since docker_publish.yml has docker/* in its paths: and docker_publish.yml has a job which runs the same job found in build_test.yml.

Copy link
Contributor

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

unless I misunderstand the paths scope, I think we want this to run more often than indicated here.

.github/workflows/build_test.yml Outdated Show resolved Hide resolved
.github/workflows/install_script.yml Outdated Show resolved Hide resolved
@bquan0
Copy link
Contributor Author

bquan0 commented Jul 27, 2023

Right, I now see that I misinterpreted the issue for install_script.yml. However, on the issue about build_test.yml, it says that the workflow shouldn't run if there are only documentation changes.

@gonuke
Copy link
Contributor

gonuke commented Jul 27, 2023

Fair enough, we can add a path exclusion for documentation on the build_test.yml

@bquan0
Copy link
Contributor Author

bquan0 commented Jul 28, 2023

Just added a list of files to paths-ignore: for build_test.yml. Let me know if there are any changes I should make to the list!

@gonuke
Copy link
Contributor

gonuke commented Jul 28, 2023

Maybe we need to revisit/talk through all the circumstances we use to run all of these at a future software meeting.

@gonuke
Copy link
Contributor

gonuke commented Aug 5, 2023

I think we need to restrict the cython installation to be <3.0. This is a new change based on which cython version is automatically being installed.

@gonuke
Copy link
Contributor

gonuke commented Aug 5, 2023

I encountered this in DAGMC recently and didn't port it over to here. May also require an update to the install script.

@bquan0
Copy link
Contributor Author

bquan0 commented Aug 5, 2023

Right now, the build_test.yml workflow is running on my fork, but it runs into errors because the docker image it tries to access if from pyne. Should I add a run condition that restricts it to only run when the owner of the repo is pyne?
After trying out a bunch of variations for installing cython but restricting its version to be <3.0, I found that pip install cython==0.* was the syntax that works (since the versions before 3.0 are numbered starting with 0.). I'm not sure why pip install cython<3.0 doesn't work, however. This is the working run.

@gonuke
Copy link
Contributor

gonuke commented Aug 5, 2023

I think you need 'cython<3' with quotation marks since the less than symbol has a special meaning on the command line

@bquan0
Copy link
Contributor Author

bquan0 commented Aug 6, 2023

Ah, makes sense. Looks like that syntax is working now! (workflow run)

@gonuke
Copy link
Contributor

gonuke commented Aug 6, 2023

Thanks @bquan0 - I think this looks ready to merge - let's try...

@gonuke gonuke merged commit 00f1666 into pyne:develop Aug 6, 2023
8 of 9 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.

Add check to avoid CI build/test for pure documentation changes
2 participants