Make Moodle Install Clone step optional

[jay-oswald]

trafficstars

I would like to propose that we make the git clone step of Moodle install an optional step.

The Rational behind this is we have a custom docker image that runs our tests on Gitlab when we push code. We would like to bake Moodle itself into the docker image. Baking in moodle, and then running moodle-plugin-ci install --moodle /moodle.

This does 2 main things

1. Speed up the test process by not having to do a fresh git clone every time a test runs.
2. Reduces load on our Gitlab server (where our Moodle fork lives), by not doing clones all the time

As it checks for if the folder exists, it will run like it does now if the folder does not exist, having no impact on people already using it.

Doing this approach allows actions to cache the location of the install, so repeated runs can all run the same command, and if the cache has been cleared it will run the git clone command, and if the cache exists the step will just be skipped

[codecov[bot]]

Codecov Report

:x: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 88.19%. Comparing base (30f6347) to head (c6a4f18). :warning: Report is 55 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Installer/MoodleInstaller.php	50.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #305      +/-   ##
============================================
- Coverage     88.23%   88.19%   -0.04%     
- Complexity      735      736       +1     
============================================
  Files            76       76              
  Lines          2270     2271       +1     
============================================
  Hits           2003     2003              
- Misses          267      268       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kabalin]

Hi @jay-oswald, thanks for the contribution. Thinking about situation when that directory could exist, but will be empty, unlikely to happen in standard setups, but may be it needs to throw exception in that case.

Also, at the current workflow you can point --repo to local directory containing existing Moodle repo clone, this will be very fast.

[jay-oswald]

Thanks for the reply @kabalin I didn't actually realise --repo can point to a folder, its not mentioned in the docs for it: https://github.com/moodlehq/moodle-plugin-ci/blob/main/docs/CLI.md I will test it out myself and do another PR to update the docs to mention it.

I still think there is a use case for making it optional when running on actions runners with caching enabled, allowing the command to be constant and it will clone or not depending on if the cache maintains folder or not. Though can probably just use the 2 commands, and have it fall back to the url in repo if using a path fails

[jay-oswald]

Hey @kabalin I just tried to use --repo pointing to a directory and it does not like it, are you sure thats an option? or am I running the command wrong? The /upstream directory has a copy of our version of Moodle, but it looks like it does not like taking a path instead of a url

$ moodle-plugin-ci install --db-host=postgres -vvv --no-interaction --repo /upstream --db-type pgsql
In Validate.php line 98:
                              
  [InvalidArgumentException]  
  Invalid URL: /upstream      
   

[stronk7]

Hi @jay-oswald,

unless I'm wrong, --repo has to be a valid git URL, so maybe (not tried, just suggesting) something like --repo file:///upstream or so is needed.

Ciao :-)

[jay-oswald]

Thanks for the suggestion @stronk7

https://github.com/moodlehq/moodle-plugin-ci/blob/main/src%2FValidate.php#L96

From what I can tell that's the function that validates the repo string that's passed in, so it needs to be git, ssh or https. So passing in file like you suggested won't work. Unless I'm wrong about the validation.

Even if it did work it would still try to run the git clone command which would throw an error at the folder not being empty.

So I think really does need to do something along the lines of my PR, I'll need to fix it up to pass tests etc

[stronk7]

Just contradicting my previous message, I think that moodle-plugin-ci does not support file:// URLs, digging into code I just found this.

And, unless I'm reading it wrong, only git/ssh/https URLs are accepted. So, if we want local repos to be allowed... we need to modify that validation.

Ciao :-)

[stronk7]

Hah, @jay-oswald, snap!

[kabalin]

It is good we did some research 👍Cloning from the path is standard git clone functionality, I assumed it would work since we use git CLI in subprocess, but I did not test :) The validation function, which @stronk7 found (thanks!) presumably prevents that. I suggest to change it to support both file:// and path ^[/|\./].

Even if it did work it would still try to run the git clone command which would throw an error at the folder not being empty.

Not sure I have got this. Cloning would use local path as remote, so it would clone from specified directory to $this->moodle->directory quick way.

I don't mind if we merge @jay-oswald patch too, kind of adds flexibility, but it needs to throw if directory is empty I guess, what do you think @stronk7 ?

[stronk7]

I suggest to change it to support both file:// and path ^[/|\./].

I fully agree with this, allowing to clone from local repos cannot but be good (for cases requiring it). I'd suggest to create different issue, because this one, though related, is a different beast, IMO.

Even if it did work it would still try to run the git clone command which would throw an error at the folder not being empty.

Not sure I have got this. Cloning would use local path as remote, so it would clone from specified directory to $this->moodle->directory quick way.

I think that what @jay-oswald is trying to achieve here is as simple as to allow the git clone operation to be completely skipped if the $this->moodle->directory directory already exists. Not to clone from local. I imagine that it's aiming to install once and save those seconds that git needs to clone (or some other workflow, say caching...).

To be honest, personally, instead of relying on directories existence (or contents, or any other heuristic...) what I'd do is to have a --no-git-clone option (name to be decided!), pretty much like the --no-init (that prevents DB initialisation) and done. So, whoever wants to skip the cloning, can do it, explicitly. If the option is used and the expected contents aren't there, it will fail later, for sure, and it's the developer responsibility to ensure that a moodle clone (or compatible dir) is there.

Ciao :-)

[kabalin]

I fully agree with this, allowing to clone from local repos cannot but be but good (for cases requiring it). I'd suggest to create different issue, because this one, though related, is a different beast, IMO.

Created #306.

I think that what @jay-oswald is trying to achieve here is as simple as to allow the git clone operation to be completely skipped if the $this->moodle->directory directory already exists. Not to clone from local. I imagine that it's aiming to install once and save those seconds that git needs to clone (or some other workflow, say caching...).

To be honest, personally, instead of relying on directories existence (or contents, or any other heuristic...) what I'd do is to have a --no-git-clone option (name to be decided!), pretty much like the --no-init (that prevents DB initialisation) and done. So, whoever wants to skip the cloning, can do it, explicitly. If the option is used and the expected contents aren't there, it will fail later, for sure, and it's the developer responsibility to ensure that a moodle clone (or compatible dir) is there.

I agree, simple and clear approach. --no-moodle-clone may be?

[jay-oswald]

@stronk7 Happy if I just update this one, and add the option, thinking just a simple --no-clone?

[stronk7]

@stronk7 Happy if I just update this one, and add the option, thinking just a simple --no-clone?

I think that the --no-moodle-clone option, as suggested by @kabalin , is a better one. Just in case tomorrow we want to avoid cloning other things, let's specify this setting is for moodle core.

Other than that, of course, you're welcome!

Ciao :-)