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 testing environment #21

Merged
merged 16 commits into from
Jul 17, 2024
Merged

Update testing environment #21

merged 16 commits into from
Jul 17, 2024

Conversation

abachma2
Copy link
Collaborator

@abachma2 abachma2 commented Jun 6, 2024

This PR updates the testing environment with two primary parts:

  • Simplifies the CI environment build process, which includes removing unused files
  • Adding a test to make sure the CHANGELOG is updated for each PR.

@pep8speaks
Copy link

pep8speaks commented Jun 6, 2024

Hello @abachma2! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-06-28 19:57:45 UTC

@abachma2 abachma2 marked this pull request as ready for review June 13, 2024 17:52
@abachma2 abachma2 self-assigned this Jun 13, 2024
@abachma2 abachma2 added Difficulty:1-Beginner This issue does not require expert knowledge and may be a good issue for new contributors. Priority:2-Normal This work is important and should be completed ASAP. Status:5-In Review This issue has been handled, and the solution is being reviewed. (probably as a PR) Type:Test Is related to software testing. Failing tests, necessary new tests, test frameworks, etc. labels Jun 13, 2024
Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @abachma2,
Good job on this PR. I have a couple things I'd like clarification on before approving.

Comment on lines +14 to +15
container:
image: alpine:3.14
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never used a container in GH actions. What's the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for running on a Docker image. I don't think this is actually needed here. I'll remove it if it isn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still curious about 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.

It looks like the docker image is needed. It's a standard Linux environment that is minimal in size: https://hub.docker.com/_/alpine/

.github/workflows/changelog_test.yml Outdated Show resolved Hide resolved
.github/workflows/changelog_test.yml Outdated Show resolved Hide resolved
.github/workflows/changelog_test.yml Outdated Show resolved Hide resolved
on:
# allows us to run workflows manually
workflow_dispatch:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will only run when a pull request is created. On subsequent pushes to that PR it will not run. Not sure if this is the behavior you want or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it will run again if you push to the branch the PR originates from:

Runs your workflow when activity on a pull request in the workflow's repository occurs. 
For example, if no activity types are specified, the workflow runs when a pull request 
is opened or reopened or when the head branch of the pull request is updated. 

from https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows

Copy link
Contributor

Choose a reason for hiding this comment

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

May want to add a push trigger

Comment on lines +25 to +27
- name: Install dependencies
run: |
pip install pytest
mamba env update -n openmcyclus-env -f environment.yml
mamba install -y cycamore openmc scipy=1.11 pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! This may not stay for long, as I'm noticing locally that when cyclus is installed locally it isn't able to find the archetype. I'll dig into this a bit more.

Copy link
Collaborator Author

@abachma2 abachma2 Jun 28, 2024

Choose a reason for hiding this comment

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

When I run the CI with the mamba build see run 129, I think it finds cyclus. So this build configuration will stay.

CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
The alpine 3.14 docker container is needed to run the ``apk``
command to install the latest version of git
@abachma2 abachma2 requested a review from yardasol June 27, 2024 16:05
Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @abachma2, good job on this PR. I still have a few questions I'd like answered before merging, but tentatively approving.

on:
# allows us to run workflows manually
workflow_dispatch:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to add a push trigger

Comment on lines +14 to +15
container:
image: alpine:3.14
Copy link
Contributor

Choose a reason for hiding this comment

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

Still curious about this

Copy link
Collaborator Author

@abachma2 abachma2 left a comment

Choose a reason for hiding this comment

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

I thought I published these comments before re-requesting a review @yardasol. My bad.

on:
# allows us to run workflows manually
workflow_dispatch:
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.

No, it will run again if you push to the branch the PR originates from:

Runs your workflow when activity on a pull request in the workflow's repository occurs. 
For example, if no activity types are specified, the workflow runs when a pull request 
is opened or reopened or when the head branch of the pull request is updated. 

from https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows

Comment on lines +25 to +27
- name: Install dependencies
run: |
pip install pytest
mamba env update -n openmcyclus-env -f environment.yml
mamba install -y cycamore openmc scipy=1.11 pytest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! This may not stay for long, as I'm noticing locally that when cyclus is installed locally it isn't able to find the archetype. I'll dig into this a bit more.

Comment on lines +14 to +15
container:
image: alpine:3.14
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for running on a Docker image. I don't think this is actually needed here. I'll remove it if it isn't.

Adding this function makes the tests more sustaiable,
as the cross section data stored in the ANL link may change
slightly with time
@abachma2 abachma2 requested a review from yardasol June 28, 2024 20:26
@abachma2
Copy link
Collaborator Author

Is there anything else for this PR? I think the timeline got a little messy because I requested a review without actually finishing my updates/responding to comments. @yardasol

@yardasol yardasol merged commit 3723a28 into arfc:main Jul 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty:1-Beginner This issue does not require expert knowledge and may be a good issue for new contributors. Priority:2-Normal This work is important and should be completed ASAP. Status:5-In Review This issue has been handled, and the solution is being reviewed. (probably as a PR) Type:Test Is related to software testing. Failing tests, necessary new tests, test frameworks, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants