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 support for limiting scope of grunt #256

Closed
PhMemmel opened this issue Nov 6, 2023 · 9 comments · Fixed by #278
Closed

Add support for limiting scope of grunt #256

PhMemmel opened this issue Nov 6, 2023 · 9 comments · Fixed by #278

Comments

@PhMemmel
Copy link
Contributor

PhMemmel commented Nov 6, 2023

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

PhMemmel commented Mar 7, 2024

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

stronk7 commented Mar 15, 2024

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:

  • Basically, this is about to ensure that the standard way that we have to ignore files and directories also works for the grunt command.
  • To implement it what we could do is to collect all the involved ignores, namely:
    • IGNORE_PATHS
    • IGNORE_NAMES
    • GRUNT_IGNORE_PATHS
    • GRUNT_IGNORE_NAMES
  • And "inject" them into the .stylelintignore and .eslintignore files
  • Then run the command, the files will be ignored without problem.
  • Finally, restitute the original ignore files, so other commands or code that may be using them is not affected.

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

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

stronk7 commented Mar 18, 2024

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

stronk7 commented Mar 18, 2024

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

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

stronk7 commented Mar 18, 2024

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

marinaglancy commented Mar 24, 2024

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

  • When running grunt check for MOODLE_403_STABLE I want to exclude vault_amd.js
  • When running grunt check for MOODLE_35_STABLE I want to exclude vault.js
  • For other Moodle versons I will skip grunt check.

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

stronk7 commented Jun 12, 2024

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants