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

Makeover for CI/CD workflows, pre-commit & MkDocs #485

Merged
merged 23 commits into from
Oct 11, 2022

Conversation

CasperWA
Copy link
Collaborator

@CasperWA CasperWA commented Oct 6, 2022

See this comment for an overview of the merging process.

Description:

Fixes #481
Fixes #482
Closes #486

CI/CD

This PR implements a complete makeover for the CI/CD workflows, utilizing the SINTEF/ci-cd callable GitHub Actions workflows as well as the pre-commit hooks there.

Benefits of this makeover:

  • Automatic updates of the underlying workflows.
  • Remove invoke dependency and tasks.py file, since these invoke tasks are replaced by the hooks and workflows from SINTEF/ci-cd.
  • Simplified workflow file contents, where only a few jobs are now explicit. Those, which are very EMMOntoPy-specific.

Possible drawbacks:

  • "Outsourcing" workflows and formerly internal invoke tasks for pre-commit hooks can lead to less flexibility if changes are needed and not implemented in the external workflows/hooks.
  • Decreased transparency. One now has to inspect the external workflows/hooks to understand exactly what is going on.

Overall, I think this is a huge improvement and should ease CI/CD immensely.

Documentation

Some updates in the MkDocs configuration were necessary to fix some presentation issues of certain documentation pages.
The table of contents (ToC) has been improved to exclude the emmodoc markdown template files under the "Examples" section.

The EMMC-CSA reference on the landing page has been updated to point to emmc.eu instead of the old emmc.info web page. Furthermore, the logo reference has been fixed to point to a local PNG image.

Type of change:

  • Bug fix.
  • New feature.
  • Documentation update.

Checklist:

This checklist can be used as a help for the reviewer.

  • Is the code easy to read and understand?
  • Are comments for humans to read, not computers to disregard?
  • Does a new feature has an accompanying new test (in the CI or unit testing schemes)?
  • Has the documentation been updated as necessary?
  • Does this close the issue?
  • Is the change limited to the issue?
  • Are errors handled for all outcomes?
  • Does the new feature provide new restrictions on dependencies, and if so is this documented?

Comments:

Use more from SINTEF/ci-cd and also determine code coverage and upload
it to codecov.io. Upload different coverages for each package.
These tasks are replaced by SINTEF/ci-cd workflows and pre-commit hooks.
Fixed the EMMC logo and link (from emmc.info to emmc.eu).
Fixed indentation in release instructions.
Fixed plugin settings for MkDocs.
Extend copyright to 2022.
@CasperWA CasperWA marked this pull request as draft October 6, 2022 13:58
@CasperWA
Copy link
Collaborator Author

CasperWA commented Oct 6, 2022

This is a draft until #486 has been resolved.

README.md Outdated Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
CasperWA and others added 2 commits October 10, 2022 11:26
Co-authored-by: Francesca L. Bleken <francesca.l.bleken@sintef.no>
@CasperWA CasperWA marked this pull request as ready for review October 10, 2022 09:28
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@ae1d33a). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #485   +/-   ##
=========================================
  Coverage          ?   62.91%           
=========================================
  Files             ?       16           
  Lines             ?     3055           
  Branches          ?        0           
=========================================
  Hits              ?     1922           
  Misses            ?     1133           
  Partials          ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

I think you suggestion of removing official support of python 3.6 is good. We have not been testing for that version until now. Alternatively, we have to rename the excel-file to .xls. I think that should work, but it is getting quite outdated.

Extend testing with Python 3.8-3.10.
Add explicit support for Python3.8-3.10 in setup.py (package) metadata.
@CasperWA
Copy link
Collaborator Author

I have removed Python 3.6 support, while extending official support to include Python 3.8, 3.9, and 3.10 (by adding these versions specifically to the metadata in setup.py - and also running the test suite with these versions.

@CasperWA
Copy link
Collaborator Author

@jesper-friis I'd like some input /review from you on this PR as well, especially regarding the supported Python version changes this will introduce (see #486 for reference).

Copy link
Collaborator

@francescalb francescalb 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 good. I am however, a little hesitant about it being much less transparent now. I suggest we open an issue about documenting a bit more.

@CasperWA
Copy link
Collaborator Author

CasperWA commented Oct 10, 2022

Information on the finalization of this PR:

  1. 2 approvals required - DONE
  2. Branch protection rules update for master (and ci/dependabot-updates) to "require" the new/updated) workflows - DONE
  3. Merge - DONE
  4. Update other existing PRs with the newly merged updates to master - DONE

@CasperWA
Copy link
Collaborator Author

This looks good. I am however, a little hesitant about it being much less transparent now. I suggest we open an issue about documenting a bit more.

That's completely fair.
I have foreseen this (slightly) and applied myself to try and write some nice documentation for the workflows and pre-commit hooks I'm using here, which are coming from the SINTEF/ci-cd GitHub repository.
The documentation can be found here.

But we should definitely extend the EMMOntoPy documentation for developers to include references to this other documentation.

@jesper-friis
Copy link
Collaborator

@jesper-friis I'd like some input /review from you on this PR as well, especially regarding the supported Python version changes this will introduce (see #486 for reference).

I agree with this suggestion.

@jesper-friis
Copy link
Collaborator

jesper-friis commented Oct 10, 2022

This looks good. I am however, a little hesitant about it being much less transparent now. I suggest we open an issue about documenting a bit more.

That's completely fair. I have foreseen this (slightly) and applied myself to try and write some nice documentation for the workflows and pre-commit hooks I'm using here, which are coming from the SINTEF/ci-cd GitHub repository. The documentation can be found here.

But we should definitely extend the EMMOntoPy documentation for developers to include references to this other documentation.

If we could improve the documentation of SINTEF/ci-cd, I think delegating to those tools is only beneficially and makes the ci/cd much easier to read. See e.g. cache for a typical example of documentation a github action.

@CasperWA
Copy link
Collaborator Author

CasperWA commented Oct 11, 2022

This looks good. I am however, a little hesitant about it being much less transparent now. I suggest we open an issue about documenting a bit more.

That's completely fair. I have foreseen this (slightly) and applied myself to try and write some nice documentation for the workflows and pre-commit hooks I'm using here, which are coming from the SINTEF/ci-cd GitHub repository. The documentation can be found here.
But we should definitely extend the EMMOntoPy documentation for developers to include references to this other documentation.

If we could improve the documentation of SINTEF/ci-cd, I think delegating to those tools is only beneficially and makes the ci/cd much easier to read. See e.g. cache for a typical example of documentation a github action.

It's important to understand that the SINTEF/ci-cd repository does not provide actions, but rather callable workflows. I.e., complete workflows with their own step-logic and actions usage.
Furthermore, it provides pre-commit hooks to autoupdate API reference documentation and the landing page when using the MkDocs framework.

I think the documentation is quite extensive as is, but everything can always be improved. Please visit the documentation here and evaluate for yourself.

@francescalb
Copy link
Collaborator

I think this looks quite OK already. Only thing is to add the documentation (link to this repo) for ontopy developers as you mention.

@CasperWA CasperWA merged commit 7920773 into master Oct 11, 2022
@CasperWA CasperWA deleted the cwa/fix-481-482-ci-cd-workflows branch October 11, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants