search

moodlehq / moodle-plugin-ci

Helps running Moodle plugins analysis checks and tests under various CI environments.
https://moodlehq.github.io/moodle-plugin-ci/
GNU General Public License v3.0
47 stars 46 forks source link

Add support for limiting scope of grunt #256

Closed PhMemmel closed 5 months ago

PhMemmel commented 1 year ago

Use case: When additional plugins needs to be installed by moodle-plugin-ci because the plugin I want to run the pipeline for depends on some other plugins, I want to run grunt and all other jobs exclusively for my plugin, NOT for the dependency plugins.

Currently however, grunt for example checks all installed plugins which means also the installed plugins just needed as dependency.

There need to be a config option (or even change the default behavior) which limits the scope to the plugin itself, not the dependency plugins.

PhMemmel commented 8 months ago

As I have not received further feedback on this I decided to create a pull request. IMO the stylelint task should be scoped to the plugin and not to the moodle directory. However, I might overlook something here. Curious to receive any feedback on this topic.

stronk7 commented 8 months ago

Hi @PhMemmel,

I've been looking to #278 and I'm not convinced that's the way to go, it's perfectly vald to test N plugins together (if they are inter-dependent or whatever).

I've been thinking about this a little bit and wanted to share if something like this would work:

While it's a little complex to achieve, it has the advantage of using the "standard" way to ignore files and dirs, can be documented and will become better if we support more commands with it (phpunit is still another one that doesn't support it, see #133 ).

How does that sound for you?

Ciao :-)

PhMemmel commented 7 months ago

Hi @stronk7,

thank you for having a look and sharing your thoughts on this.

While I basically agree that it's perfectly ok that you want moodle-plugin-ci to test multiple plugins, I do not think this is the basic requirement and it's not what you would expect moodle-plugin-ci to do "when just running it on your plugin repository". All other jobs that moodle-plugin-ci is doing are targeting the plugin you are running it for: Unit tests are only run for the plugin, phpcs, ..., even grunt amd for example just uses the plugin directory and does not rebuild all the js files of the other plugins (and yes, why would it supposed to do this, we are running the moodle-plugin-ci for this plugin). Only grunt stylelint currently does this thing of checking other plugins as well which does not feel like it's intended. Also these additional plugins usually are dependencies which you have to add to the pipeline to make your plugin work, so you usually do not even have control of these dependencies and you do not care either. You know your plugin depends on it, but you definitely do not want the moodle-plugin-ci runs of your plugin to be influenced by these libraries.

So I think at least the default should be to only run the stylelint task for the current plugin.

Your suggestions about enhancing grunt to be able to ignore files of course should be pursued as well, but IMO it's kind of another issue.

stronk7 commented 7 months ago

Just linking this with https://tracker.moodle.org/browse/MDL-81263 that would fix this automatically, and in the more natural way, I would say.

stronk7 commented 7 months ago

So I think at least the default should be to only run the stylelint task for the current plugin.

Uhm, let me think about this a little more… maybe you’re ok, but I want to see how all the commands are behaving right now…

Thanks!

PhMemmel commented 7 months ago

Sure, there is a good chance I might misunderstand something here or missing out on an already existing option or sth. Thanks again! :)

BTW: I do not see why https://tracker.moodle.org/browse/MDL-81263 would fix this automatically, but I can see that this could be used to implement something like you suggested. MDL-81263 is about being able to ignore files from the plugin I'm currently running the moodle-plugin-ci for, this issue here however is about ignoring files from OTHER plugins I have to add to the pipeline because of the fact that my plugin is depending on them.

stronk7 commented 7 months ago

Yeah, not automatically, sorry I was too much excited (and having breakfast, aka sleepy), LOL.

I just imagined that people would adopt that (distributed ignore files), to add whatever shouldn't be checked in their plugins and that would, ultimately, lead to the "real" plugin being checked not failing because of dependencies.

But yes, it won't fix this issue by default, just add a new way to avoid problems sometimes.

Ciao :-)

marinaglancy commented 7 months ago

Hi! I was about to report a new issue but found this one.

Here is my use case: I'm trying to make a single version of the plugin that works on all Moodle versions from very ancient (I went back as far as 3.5) as well as 4.3 and 'main'.

The problem I found is that I can not use ES6 in the old Moodle versions, so I have to package two types of JS files - AMD modules and ES6 modules. I have to compile the "AMD" ones separately using older node/grunt.

So basically, I want to run grunt check in moodle-plugin-ci, because it is useful but I want to exclude some JS files.

Let's say I include two files: vault.js and vault_amd.js

P.S. Right now I'm considering the workaround - add a manual step in the GHA file that removes these JS files completely before running moodle-plugin-ci checks

stronk7 commented 5 months ago

Hi @PhMemmel,

today (at last!) I've been able to spend a few time with this and reviewing how other grunt tasks behave by default.

So, I think that it's correct to restrict the stylelint tasks to only operate on the plugin dir, and not in moodleroot (that implies both the whole Moodle and any other "secondary" plugin that may be installed. That's the way all the tasks (but the gherkinlint one that is always global) behave.

So, I've added a few changes to #278 (style details, changelog, amend one test) and it's being checked now.

About the alternative of adding support for IGNORE files and paths, some day we should make them to work with all the commands, but now / here it's not the time for that. Let's fix this issue and see if there is any request about that new feature elsewhere.

And yes, @marinaglancy, I imagine that, right now, the only way to keep both JS alternatives without branching the plugin is to apply for a custom removal before executing moodle-plugin-ci. If you think that trying to implement the IGNORE thingy would help, a new issue is welcome.

Ciao :-)