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

fix for CI (black) #78

Merged
merged 2 commits into from
Apr 21, 2024
Merged

fix for CI (black) #78

merged 2 commits into from
Apr 21, 2024

Conversation

tomsail
Copy link
Contributor

@tomsail tomsail commented Apr 5, 2024

To solve #77

I am first putting as draft to if CI passes

@krober10nd
Copy link
Collaborator

Seems like there were some issues with starting up the CI system with versions of Python other than 3.9?

@pmav99
Copy link

pmav99 commented Apr 8, 2024

The reason this fails is because you are asking black to automatically fix the issues it finds and it seems that there are permissions issues when it tries to amend the commits. You can either:

  1. Try to fix the permissions issues: https://github.com/CHLNDDEV/oceanmesh/actions/runs/8567158490/job/23478362132?pr=78#step:5:89
  2. Use a flag like black --check or black --diff which will fail if there are differences without trying to make changes to the source code. Not sure how you do that with wearerequired/lint-action@master but the docs should mention something.

@tomsail
Copy link
Contributor Author

tomsail commented Apr 8, 2024

So there are various problems here:

  1. the formatter CI: the permissions issue with wearerequired/lint-action@master does not work for forks. Issue is still open on wearerequired page. We might want to test in a different way.
  2. sudo apt install -y libopenmpi3 libopenmpi-dev openmpi-bin failing for some python versions. example here
  3. black version issue. I will try to see which version throws the least number of errors. I had started to test (as you can see in the commit), but didn't finish it.

@tomsail
Copy link
Contributor Author

tomsail commented Apr 8, 2024

the formatter CI: the permissions issue with wearerequired/lint-action@master does not work for forks. Issue is still wearerequired/lint-action#13 on wearerequired page. We might want to test in a different way.

It works anyway if black and flake go through. Although permissions are still an issue (see last logs), if linting is fixed it won't raise any error at the end.

sudo apt install -y libopenmpi3 libopenmpi-dev openmpi-bin failing for some python versions. example here

Fixed by adding "sudo apt update in the CI steps"

black version issue. I will try to see which version throws the least number of errors. I had started to test (as you can see in the commit), but didn't finish it.

Fixed by removing:

line-length = 108

and letting default maximum line-length

@tomsail tomsail marked this pull request as ready for review April 8, 2024 15:59
@tomsail
Copy link
Contributor Author

tomsail commented Apr 8, 2024

should be all good now. let's wait to see if latest CI pass

@krober10nd
Copy link
Collaborator

Excellent thank you Thomas.

@krober10nd krober10nd self-requested a review April 8, 2024 23:10
@@ -12,7 +12,7 @@ jobs:
steps:
- uses: actions/setup-python@v2
with:
python-version: "3.x"
python-version: "3.7"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the 3.x imply that many versions of Python would be tested? Is it possible to test the last three trailing versions minus the current (which is typically always problematic)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a sort of "wildcard" that matches the latest minor python3 version in github CI.
Since @pmav99 suggested to use the lowest available, I changed it to 3.7

@tomsail
Copy link
Contributor Author

tomsail commented Apr 9, 2024

@krober10nd is it really necessary to have the formatter.yml CI action?
It seems a bit overkill to use and deploy wearerequired/lint-action@master (which is itself a library that can induce changes) for just the linting (that we already have in the test CI action.

@krober10nd
Copy link
Collaborator

it is not essential since the PR author will know if it is violating the format. I'm in favor of removing it.

@krober10nd krober10nd merged commit a017890 into CHLNDDEV:master Apr 21, 2024
5 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.

3 participants