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

Add composite action for BuildTest job #1489

Merged
merged 13 commits into from
Jul 14, 2023
Merged

Conversation

bquan0
Copy link
Contributor

@bquan0 bquan0 commented Jul 10, 2023

Description

Adds a composite action to replace the BuildTest job used in docker_publish.yml and build_test.yml.

Motivation and Context

Requested by @gonuke. This change will keep the BuildTest jobs in the two workflows consistent and requires less effort to maintain the consistency.

Behavior

Current behavior is that the BuildTest jobs in docker_publish.yml and build_test.yml have become inconsistent with each other due to previous pull requests I made where I only updated the BuildTest job in docker_publish.yml. Now, I have put the BuildTest job from docker_publish.yml (the most updated version) into the composite action and it is being used in both workflows.

Other Information

The composite action can only include the steps of the BuildTest job, which means that the matrix and the container of the BuildTest job still need to be updated in their respective workflow files. However, the container of the BuildTest job already had differences between the two workflow files in terms of their tag, so this issue wouldn't have been fixed with the composite action anyway.
Here's a workflow run to show that the BuildTest job still functions as intended with the composite action.

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.

Thanks for this @bquan0 . I've included a few suggestions to improve documentation, but also think this would be a good to clean up a few issues that are not part of these changes (and therefore can't be places where I can place comments).

  • we don't use the matrix variable ubuntu_versions in the docker_publish action
  • the use of output and/or env variables for tag names seems a little random and may need to be cleaned up in docker_publish
  • might want more descriptive id for the two builds in docker_publish

.github/actions/build-test/action.yml Outdated Show resolved Hide resolved
.github/actions/build-test/action.yml Outdated Show resolved Hide resolved
.github/actions/build-test/action.yml Outdated Show resolved Hide resolved
cd $GITHUB_WORKSPACE
python setup.py install --user --clean ${{ env.ADD_FLAG}}
export PATH="$PATH:/github/home/.local/bin"
export PYTHONPATH="$PYTHONPATH:/github/home/.local/lib/python3.10/site-packages/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if this is necessary? If so, how do we make it more robust to changing python versions?

@@ -34,41 +34,15 @@ jobs:
fail-fast: false

container:
image: ghcr.io/pyne/pyne_ubuntu_20.04_py3${{ matrix.hdf5 }}/${{ matrix.stage }}:stable
image: ghcr.io/pyne/pyne_ubuntu_22.04_py3${{ matrix.hdf5 }}/${{ matrix.stage }}:stable
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we make sure this is consistent with the docker images we've built? Maybe there is no good way to automate this? Or maybe we just take some of the modifiers out of the image name (e.g. ubuntu version). This could be a good topic for a software meeting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a good way to achieve consistency because env variables can't be used in image:. Furthermore, there's no way to ensure consistency across docker_publish.yml and build_test.yml because image: can't be in the composite action. We do have the option to remove the ubuntu version from the image name since that's something that we specify in the env variable DOCKER_IMAGE_BASENAME.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - I was wondering we can just do without the ubuntu version given the streamlining we are doing. We can probably remove the py3 as well since we are always using Python 3 now. In short, maybe all identifiers should be machine-generated rather than hard coded.

@bquan0
Copy link
Contributor Author

bquan0 commented Jul 10, 2023

The use of output and env variables for tag names is currently necessary in order to test and push the images that we build in the first job of docker_publish.yml because we need to have the tag the action generates in order to use the correct image. The tag-latest-on-default option doesn't help here because it only gives a latest tag if the workflow is run on the default branch.

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.

This looks better now. I'll check the builds and then merge as-is. We can revisit the change of the image name more carefully since we probably need to modify it in two separate steps, starting with the docker_publish workflow first.

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.

I found some warnings in the github builds around these extra inputs

.github/workflows/docker_publish.yml Outdated Show resolved Hide resolved
.github/workflows/build_test.yml Outdated Show resolved Hide resolved
@gonuke
Copy link
Contributor

gonuke commented Jul 14, 2023

Thanks @bquan0 - let's put this into production

@gonuke gonuke merged commit fe0a590 into pyne:develop Jul 14, 2023
6 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.

None yet

2 participants