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.

Sign up for GitHub

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

Jump to bottom

2023 build fixes #549

Merged
merged 10 commits into from
Nov 13, 2023
Merged

2023 build fixes #549

merged 10 commits into from
Nov 13, 2023

Conversation

bennibbelink
Copy link
Contributor

Replaces #548 on an upstream branch.

@bennibbelink bennibbelink mentioned this pull request Nov 12, 2023
Closed
@bennibbelink bennibbelink linked an issue Nov 12, 2023 that may be closed by this pull request
2023 build fixes #543
Closed
@bennibbelink bennibbelink marked this pull request as draft November 12, 2023 15:00
@bennibbelink
Copy link
Contributor Author

bennibbelink commented Nov 12, 2023 •
edited
Loading

I'm not sure how to get rid of the circleCI checks, I'm guessing they won't go away until circle.yml is deleted in main.

Also - I'm marking this as draft for now because I want to play around with the CI environment so that these warnings go away:

WARNING: Error loading config file: /root/.docker/config.json: open /root/.docker/config.json: permission denied

Right now the workflows set /root as the home directory to conform to the cyclus docker images we use, but GitHub's CI environment doesn't seem to like that. It doesn't stop the workflows from passing, but I'd rather nip this here than run into problems down the line

@gonuke
Copy link
Member

gonuke commented Nov 12, 2023

I can also change the repo settings and turn off the CircleCI web hook

@bennibbelink
resolve docker config errors in workflows
65c5d08 
squash 10 debugging commits
@bennibbelink
Copy link
Contributor Author

Resolved the error, PR is ready for review now

@bennibbelink bennibbelink marked this pull request as ready for review November 13, 2023 01:27
gonuke
gonuke requested changes Nov 13, 2023
View reviewed changes
Copy link
Member

@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 pushing this forward - just one little request.

.github/workflows/build_test.yml Outdated Show resolved Hide resolved
tests/test_regression.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if all this classmethod cleanup was necessary or just good practice, but I like it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there were a few sound ways to go about it. Using classmethods looked to me like the easiest transition from our inheritance-based test setup, but pytest also supports unittest based tests - something to keep in mind I guess

tests/test_run_inputs.py Outdated Show resolved Hide resolved
gonuke
gonuke approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@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 @bennibbelink - I'll monitor the tests and merge when completed (successfully)

@gonuke gonuke merged commit 038239e into main Nov 13, 2023
12 of 13 checks passed
@nuclearkatie nuclearkatie mentioned this pull request Nov 27, 2023
Merged
1 task
@bennibbelink bennibbelink deleted the 2023-build-fixes branch February 16, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonuke gonuke gonuke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2023 build fixes
2 participants
@bennibbelink @gonuke