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

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

Closed
CodeWithEmad opened this issue Oct 1, 2023 · 12 comments · Fixed by #155
Closed

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

CodeWithEmad opened this issue Oct 1, 2023 · 12 comments · Fixed by #155
Assignees
Labels
enhancement New feature or request

Comments

@CodeWithEmad
Copy link
Member

CodeWithEmad commented Oct 1, 2023

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?

@CodeWithEmad CodeWithEmad changed the title Add Makefile to old plugins Add Makefile, Linting, and Testing to old plugins Oct 1, 2023
@regisb
Copy link
Contributor

regisb commented Oct 2, 2023

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.

@regisb regisb added the enhancement New feature or request label Oct 2, 2023
@CodeWithEmad
Copy link
Member Author

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
Copy link
Contributor

regisb commented Oct 2, 2023

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
Copy link
Member Author

CodeWithEmad commented Oct 2, 2023

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

@CodeWithEmad
Copy link
Member Author

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
Copy link
Member Author

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

@regisb
Copy link
Contributor

regisb commented Oct 2, 2023

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
Copy link
Member Author

CodeWithEmad commented Nov 18, 2023

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:
overhangio/tutor-credentials#16
overhangio/tutor-discovery#54
overhangio/tutor-notes#31
overhangio/tutor-minio#32
overhangio/tutor-ecommerce#50
overhangio/tutor-cairn#19
overhangio/tutor-android#13
overhangio/tutor-forum#29
overhangio/tutor-webui#9
overhangio/tutor-indigo#50
overhangio/cookiecutter-tutor-plugin#25
#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
Copy link
Contributor

regisb commented Nov 21, 2023

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

@CodeWithEmad
Copy link
Member Author

Absolutely! My pleasure.

@CodeWithEmad
Copy link
Member Author

@CodeWithEmad
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

2 participants