Add support for post_get_sources_script, replace deprecated pre_clone_script with pre_get_sources_script

[fkleon]

• Add support for post_get_sources_script in the runner config.
• The pre_clone_script config option has been deprecated for a while now and is replaced by pre_get_sources_script.

See GitLab 16.2 docs: Advanced Configuration / The [[runners]] section

[guenhter]

LGTM.

@fkleon I think we should somehow manage old pre_clone_script variables in case people are not aware of this change. Maybe we just let this role fail in case the old variable is still defined... What do you think?

[riemers]

Or just push a release and add a note of a breaking change. Microsoft does it all the time ;) Verzonden vanuit Outlook voor iOS<https://aka.ms/o0ukef>

…

________________________________ Van: Günther Grill ***@***.***> Verzonden: Tuesday, August 15, 2023 7:23:31 PM Aan: riemers/ansible-gitlab-runner ***@***.***> CC: Subscribed ***@***.***> Onderwerp: Re: [riemers/ansible-gitlab-runner] Add support for `post_get_sources_script`, replace deprecated `pre_clone_script` with `pre_get_sources_script` (PR #279) LGTM. @fkleon<https://github.com/fkleon> I think we should somehow manage old pre_clone_script variables in case people are not aware of this change. Maybe we just let this role fail in case the old variable is still defined... What do you think? — Reply to this email directly, view it on GitHub<#279 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAFGQOKSJOQP2CBGFJLUXQTXVOWBHANCNFSM6AAAAAA3QNFU7Y>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[fkleon]

Thanks for reviewing this!

I thought about backwards-compatibility but wasn't sure what your preferred way of implementing this is.

There are a few options - and of course it also depends on the GitLab runner version the user decides to install (for which I believe there are no checks in this role yet).

• Maintain backwards compatibility (at least until GitLab drops the field in question): Keep support for pre_clone_script and print a warning about use of a deprecated field.
• Maintain backwards compatibility (longer term): Use the value of pre_clone_script to populate pre_get_sources_script internally and print a warning about use of a deprecated field.
• Break backwards compatibility (loudly): Print a warning or fail the role if the deprecated field is used.
• Break backwards compatibility (silently): As implemented; deprecated field will just be ignored.

@guenhter As a user of this Ansible role I'd probably prefer this to break loudly (instead of silently) together with a breaking change notice in the changelogs. I can add this to the MR if you're happy with that approach.

[guenhter]

I'm happy with every approach. If you just add one this is fine.

[fkleon]

Hi @guenhter and @riemers, I've added a commit that adds basic runner config validation via assert. For now the only setting that it picks up is the use of pre_clone_script. When this is detected, the execution fails with an appropriate error message telling the user to switch to the new variable name.

In the future that could be extended to perform additional validation; or to alert the user to upcoming deprecations.

[fkleon]

@guenhter Just checking whether you've got any thoughts on this latest addition?

[guenhter]

For me it's fine. @riemers any objections?

[riemers]

LGTM, if your good just merge it

[guenhter]

@fkleon thanks for your contribution