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

ci: Reorganize CI #362

Merged
merged 8 commits into from Jan 25, 2024
Merged

ci: Reorganize CI #362

merged 8 commits into from Jan 25, 2024

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Nov 13, 2023

  • Rename the CI jobs so that they are more legible in the report
  • Fix concurrency on the imported jobs
  • Separated pre-commit to its own step
  • Add a static-analysis checker. I was experimenting with sonarcloud at CMake-Template repo. I think it could be useful to include it as an intermediate to clang-tidy. It is also polyglot, so it might pick up on more nieche problems. Postponed to Add a static-analysis checker #400
  • Mask overall CI status on main branch
  • Badges!!!

Depends on: #361 #358

@LecrisUT LecrisUT self-assigned this Nov 13, 2023
@LecrisUT LecrisUT force-pushed the ci/organize branch 2 times, most recently from 737a0ae to e7c6073 Compare November 14, 2023 10:53
@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c945bae) 83.85% compared to head (5cc5eb5) 83.85%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #362   +/-   ##
========================================
  Coverage    83.85%   83.85%           
========================================
  Files           24       24           
  Lines         8181     8181           
========================================
  Hits          6860     6860           
  Misses        1321     1321           
Flag Coverage Δ
c_api 77.59% <ø> (ø)
fortran_api 56.14% <ø> (ø)
python_api 80.38% <ø> (ø)
unit_tests 1.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT changed the title ci: Cleanup CI ci: Reorganize CI Jan 19, 2024
README.md Show resolved Hide resolved

![License Status][license-badge]
![CMake Status][cmake-badge]
[![Python Versions][python-version]][pypi-link]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Python version badge will be fixed automatically in next pypi release

README.md Show resolved Hide resolved
@lan496
Copy link
Member

lan496 commented Jan 19, 2024

@LecrisUT I've received "Request to install Codecov in spglib". Is it needed for the codecov badge? I remember this project is already integrated with codecov. What is the difference?

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jan 19, 2024

@LecrisUT I've received "Request to install Codecov in spglib". Is it needed for the codecov badge? I remember this project is already integrated with codecov. What is the difference?

Can you get the badge api-token without installing codecov app? That's the main thing I'm trying with that.

@lan496
Copy link
Member

lan496 commented Jan 20, 2024

Do you need the badge api token as GitHub's Action secrets or raw string?

@LecrisUT
Copy link
Collaborator Author

Do you need the badge api token as GitHub's Action secrets or raw string?

Raw string, it's meant to be public and used in the README.md

@lan496
Copy link
Member

lan496 commented Jan 20, 2024

I see. Then, here it is.

[![codecov](https://codecov.io/gh/spglib/spglib/graph/badge.svg?token=G91xAWu1JT)](https://codecov.io/gh/spglib/spglib)

@LecrisUT LecrisUT force-pushed the ci/organize branch 3 times, most recently from e186729 to b749111 Compare January 20, 2024 01:17
@LecrisUT LecrisUT marked this pull request as ready for review January 20, 2024 11:29
@LecrisUT LecrisUT force-pushed the ci/organize branch 6 times, most recently from 5d81a44 to 67261a4 Compare January 20, 2024 13:21
@LecrisUT
Copy link
Collaborator Author

All badges except for Python version are working. Just 2 things for this PR:

  • Feedback on namings:
    • I am considering CI / 📘 docs / Sphinx-html instead, but would that be confusing?
    • Would 🧪 be sufficient to indicate experimental jobs?
    • + sign is excessive?
    • Other suggestions for emojis?
  • When ready to merge, the pass job changed name a bit.

@LecrisUT LecrisUT assigned lan496 and unassigned LecrisUT Jan 20, 2024
@lan496
Copy link
Member

lan496 commented Jan 24, 2024

Mask overall CI status on main branch

Can you explain this a little more? Are these badges reflected only on main branch?

@lan496
Copy link
Member

lan496 commented Jan 24, 2024

Feedback on namings:

I think the current emojis are nice!

@LecrisUT
Copy link
Collaborator Author

Mask overall CI status on main branch

Can you explain this a little more? Are these badges reflected only on main branch?

It's easiest with an example:
If you open the CI checks for this PR, you will see windows is failing, but the merge commit is showing it as passing. That is because I am masking the true result there so that on the main page the first commit is shown with a check mark.

README.md Outdated Show resolved Hide resolved
toolchain: [ gcc, llvm, intel, windows, macos ]
python-version: [ "3.8", "3.9", "3.10", "3.11", "3.12"]
toolchain: [ gcc, llvm, intel ]
python-version: [ "3.8", "3.x" ]
Copy link
Member

Choose a reason for hiding this comment

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

@lan496
Copy link
Member

lan496 commented Jan 24, 2024

I see. Thanks, I've left a few comments.

@lan496
Copy link
Member

lan496 commented Jan 24, 2024

Add a static-analysis checker. I was experimenting with sonarcloud at CMake-Template repo. I think it could be useful to include it as an intermediate to clang-tidy. It is also polyglot, so it might pick up on more nieche problems

Is it included?

@LecrisUT
Copy link
Collaborator Author

Add a static-analysis checker. I was experimenting with sonarcloud at CMake-Template repo. I think it could be useful to include it as an intermediate to clang-tidy. It is also polyglot, so it might pick up on more nieche problems

Is it included?

Not yet, I will postpone it for after release #400


[ci-badge]: https://github.com/spglib/spglib/actions/workflows/ci.yaml/badge.svg?branch=develop&event=push
[ci-link]: https://github.com/spglib/spglib/actions/workflows/ci.yaml?query=branch%3Adevelop+event%3Apush
[cmake-badge]: https://img.shields.io/badge/CMake-3.15-blue?logo=data:image/svg%2bxml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIGRhdGEtbmFtZT0iTGF5ZXIgMiIgdmlld0JveD0iMCAwIDU0NS41IDU0NS45Ij48cGF0aCBkPSJNNTQ2IDUzNCAyODIgOWwzOSA0MzQgMjI1IDkxeiIgZGF0YS1uYW1lPSJyZWQgcmlnaHQiIHN0eWxlPSJmaWxsOiNmMzI3MzU7c3Ryb2tlLXdpZHRoOjAiLz48cGF0aCBkPSJNNTI1IDU0NiAxNzAgNDAzIDEgNTQ2aDUyNHoiIGRhdGEtbmFtZT0iZ3JlZW4gYm90dG9tIiBzdHlsZT0ic3Ryb2tlLXdpZHRoOjA7ZmlsbDojM2VhZTJiIi8+PHBhdGggZD0iTTI2MyAwIDAgNTIybDI4OC0yNDRMMjYzIDB6IiBkYXRhLW5hbWU9ImJsdWUgbGVmdCIgc3R5bGU9ImZpbGw6IzAwNjhjNztzdHJva2Utd2lkdGg6MCIvPjxwYXRoIGQ9Im0yOTEgMjk5LTEwNSA4OSAxMTcgNDgtMTItMTM3eiIgZGF0YS1uYW1lPSJncmF5IGNlbnRlciIgc3R5bGU9ImZpbGw6I2RjZTNlYztzdHJva2Utd2lkdGg6MCIvPjwvc3ZnPg==
Copy link
Member

Choose a reason for hiding this comment

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

Note: We have to manually update the badge for the CMake minimum version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, specifically only the 3.15 text and only when we bump minimum version

@lan496
Copy link
Member

lan496 commented Jan 25, 2024

@LecrisUT Can you address the branch conflicts?

LecrisUT and others added 8 commits January 25, 2024 11:40
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Concurrency should be checked only on the top-level CI/caller

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Make the CI report more navigable

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
- Reduce python tests: Only minimal + latest + future python version
- Fix os toolchains

Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
@LecrisUT
Copy link
Collaborator Author

@LecrisUT Can you address the branch conflicts?

@lan496 Done. I forgot, you need to switch the required CI for the auto-merge

@lan496 lan496 disabled auto-merge January 25, 2024 11:48
@lan496 lan496 merged commit ddbdd40 into spglib:develop Jan 25, 2024
31 of 35 checks passed
@lan496
Copy link
Member

lan496 commented Jan 25, 2024

Current branch rules
image

@LecrisUT LecrisUT deleted the ci/organize branch January 25, 2024 14:03
@LecrisUT LecrisUT added this to the 2.3 milestone Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants