search

overhangio / tutor-mfe

This plugin makes it possible to easily add micro frontend (MFE) applications on top of an Open edX platform that runs with Tutor.
GNU Affero General Public License v3.0
22 stars 95 forks source link

Add Makefile, Linting, and Testing to old plugins #151

Closed CodeWithEmad closed 1 year ago

CodeWithEmad commented 1 year ago

Hi @arbrandes It is essential to update old plugins to match the structure of new ones as cookiecutter-tutor-plugin evolves. I have noticed that the tutor-mfe plugin, along with other old plugins, does not have a Makefile. As a result, the processes of linting and testing were not carried out. If this is not intentional, I would like to update all of the old plugins. since this will affect multiple repos, I'll ping @regisb and @kdmccormick too.

ps: I see a pre-commit-config.yaml in the tutor-credentials. since all plugins should be in sync, should we consider removing the pre-commit config file?

regisb commented 1 year ago

It is essential to update old plugins to match the structure of new ones as cookiecutter-tutor-plugin evolves.

I'm not sure of that. I would definitely not call it "essential". But I agree that there are some features from the cookiecutter that would be super useful in other plugins, including linting and testing.

Making the same changes in many plugins at once is a little painful for non core contributors. I have a convenient local script that allows me to apply the same changes to many repos (it's basically tmux with set-window-option synchronize-panes on). If you agree I can make the changes myself?

ps: I see a pre-commit-config.yaml in the tutor-credentials. since all plugins should be in sync, should we consider removing the pre-commit config file?

The credentials plugin was moved from its original author @lpm0073. I believe we can remove this file.

CodeWithEmad commented 1 year ago

I would definitely not call it "essential".

Yes, you're absolutely right. "Useful" is better.

If you agree I can make the changes myself?

I have made some changes to a few repositories on my local. To help you with adding the Makefile, using tmux with 'synchronize-panes on' would be helpful but it won't be enough to ensure that running 'make test' will succeed. This is mostly due to the 'test-type'. If you agree, I would be happy to work on this for all repositories.

regisb commented 1 year ago

If you agree, I would be happy to work on this for all repositories.

Please do! But let's do one complete PR first. In particular, it would be great if you could:

  1. Make sure that make test is run on GitHub pull requests.
  2. Add make changelog-entry and make changelog targets to the plugin makefiles.
CodeWithEmad commented 1 year ago

Okay. do you have any particular repos in mind? tutor-mfe is a good candidate.

CodeWithEmad commented 1 year ago

Quick question: should we have a requirements/test.txt or requirements-test.txt with test-related packages (black, mypy, isort, ...) and use that in the test.yml file or they should be installed one by one (which I'm not a fan of) inside of the test.yml? if we go with the first option, should we consider having the requirements file in the cookiecutter-tutor-plugin repo too?

CodeWithEmad commented 1 year ago

@regisb I think you saw the question, but forgot to answer it :))

regisb commented 1 year ago

We should keep the plugins and cookiecutter as lean as possible, so let's not add requirements files. We can assume that users will install development requirements from tutor.

CodeWithEmad commented 1 year ago

OK. so since we have the Makefile and a test action, running on tutor-mfe, I've added the functionality to all other overhang plugins: https://github.com/overhangio/tutor-credentials/pull/16 https://github.com/overhangio/tutor-discovery/pull/54 https://github.com/overhangio/tutor-notes/pull/31 https://github.com/overhangio/tutor-minio/pull/32 https://github.com/overhangio/tutor-ecommerce/pull/50 https://github.com/overhangio/tutor-cairn/pull/19 https://github.com/overhangio/tutor-android/pull/13 https://github.com/overhangio/tutor-forum/pull/29 https://github.com/overhangio/tutor-webui/pull/9 https://github.com/overhangio/tutor-indigo/pull/50 https://github.com/overhangio/cookiecutter-tutor-plugin/pull/25 https://github.com/overhangio/tutor-mfe/pull/165

since this issue is closed, isn't it better to have a task in DevEx WG board and track the progress from there? Also, only tutor-mfe and tutor-android will run the jobs automatically and other plugins need approval.

regisb commented 1 year ago

@CodeWithEmad since you're on a roll, can I ask that you open a similar PR for the jupyter plugin? :innocent:

CodeWithEmad commented 1 year ago

Absolutely! My pleasure.

CodeWithEmad commented 1 year ago

https://github.com/overhangio/tutor-jupyter/pull/3

CodeWithEmad commented 12 months ago

Also, I haven't forgotten about the XQueue plugin, but I had some issues with typing. https://github.com/overhangio/tutor-xqueue/pull/28