Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5908

cfiehe · 2023-01-28T11:56:36Z

SUMMARY

This fix introduces the new boolean option access_level_on_creation. It controls, whether the value of access_level is used for runner registration or not. The option access_level has been ignored on registration so far and was only used on updates. The user is informed by a deprecation warning, if the option is unspecified. For reasons of compatibility false is assumed in that case. The option access_level_on_creation will switch to true in community.general 7.0.0.

Signed-off-by: Christoph Fiehe [email protected]

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

gitlab_runner

ansibullbot · 2023-01-28T11:58:40Z

cc @Lunik @SamyCoenen @Shaps @dj-wasabi @lgatellier @marwatk @metanovii @nejch @scodeman @sh0shin @suukit @waheedi @zanssa
click here for bot help

nejch · 2023-01-28T12:45:16Z

plugins/modules/gitlab_runner.py

@@ -216,6 +216,7 @@ def create_or_update_runner(self, description, options):
                'locked': options['locked'],
                'run_untagged': options['run_untagged'],
                'maximum_timeout': options['maximum_timeout'],
+                'access_level': options['access_level'],


This looks ok to me, but I think it has a slight change in runner registration behavior: this module defines ref_protected as the default for creation, which I think is not the real default in GitLab's API if the attribute is not provided*.

community.general/plugins/modules/gitlab_runner.py

Line 331 in 13b9d94

access_level=dict(type='str', default='ref_protected', choices=["not_protected", "ref_protected"]),

But I guess this happens anyway after the first run with the current approach, since the update method will then change it, and that is the reason you opened the issue in the first place I assume.

Just wanted to point this out but will let @felixfontein decide on the best approach.

*See https://docs.gitlab.com/ee/api/runners.html#register-a-new-runner and https://docs.gitlab.com/runner/register/

For a protected runner, use the --access-level="ref_protected" parameter. For an unprotected runner, use --access-level="not_protected" instead or leave the value undefined.

A little side note: Not sure about the history behind this, but I think selecting ref_protected as the default here was a bit of an exotic choice. Of course that's more secure as it limits the runner to being available on protected branches only, but I'm not sure that's the more common use case for GitLab runners.

That is good remark. I was not aware of that fact. From that point of view, it would be the best to also change the default value of access_level to not_protected. Because the access_level was not passed to the runner creation call before, people actually got an unprotected runner. That is not what you would expect from the documentation. But it is inline with the default behavior of the GitLab API.

@cfiehe I agree that makes more sense and I see you've already incorporated it. The only thing now is that this is technically a breaking change so I'm eager to hear feedback from the collection maintainers (I'm only involved in the GitLab module here :))

Changing the default value is a breaking change and cannot be merged (see also #582). Changing the default requires a longer deprecation period.

Generally I would avoid having a default at all though, then the default on creation would be "use whatever GitLab thinks is the default", and the default for updating would be "don't update that value". Whether you prefer to force a specific value for this option if none is provided, or whether not specifying a value means "I don't care / don't change", that's something for the module maintainers to decide. But it's nothing this bugfix PR should touch/change.

@felixfontein the issue is that the bugfix, without removing the default, would also be a breaking change.

So maybe removing the default here is the easiest transition, but then the code will need to ensure that it's not sending null values to the API if no attribute is present, as GitLab sometimes doesn't like that.

That's true, but at least it's restoring the documented behavior, and not changing the documented behavior and the implementation.

But yeah, it's also not helping that the current behavior has been there for quite a few years (it has always been like this since it was migrated to this repository). That's also (in part) why I didn't add the backport-5 label.

So maybe the best way forward is:

Add a new option access_level_on_creation of type boolean with no explicit default, but implicit default false.

If that option is not specified, a deprecation warning will be triggered that this option will switch to true for the next major release (community.general 7.0.0).

Only set the access level on creation when access_level_on_creation=true is passed to the module.

It would be a rather short deprecation period, but at least users would know that something is changing, and (most importantly) can get the module to use the new behavior already now. What do you think about this approach?

(Deprecating the default value of access_level is something that should probably happen in a separate PR, as it's not strictly related to this problem.)

github-actions · 2023-01-28T14:19:01Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

changelogs/fragments/5907-fix-gitlab_runner-not-idempotent.yml

plugins/modules/gitlab_runner.py

changelogs/fragments/5907-fix-gitlab_runner-not-idempotent.yml

plugins/modules/gitlab_runner.py

changelogs/fragments/5907-fix-gitlab_runner-not-idempotent.yml

cfiehe · 2023-01-29T17:36:46Z

Thanks for the review. I have added the requested changes and rebased the branch.

felixfontein

Besides this looks good to me! @nejch what do you think?

plugins/modules/gitlab_runner.py

…rst run after runner creation This fix introduces the new boolean option 'access_level_on_creation'. It controls, whether the value of 'access_level' is used for runner registration or not. The option 'access_level' has been ignored on registration so far and was only used on updates. The user is informed by a deprecation warning, if the option is unspecified. For reasons of compatibility 'false' is assumed in that case. The option 'access_level_on_creation' will switch to 'true' for the next major release (community.general 7.0.0) Signed-off-by: Christoph Fiehe <[email protected]>

nejch

Besides this looks good to me! @nejch what do you think?

@felixfontein @cfiehe LGTM like this! It might make sense to track the access_level default in a follow-up to align with GitLab eventually, or @cfiehe if you're willing to open another PR for that :)

patchback · 2023-01-30T20:03:17Z

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/31ff3f662d91fc36e453415ccb229282b479f96c/pr-5908

Backported as #5922

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

…r creation (#5908) This fix introduces the new boolean option 'access_level_on_creation'. It controls, whether the value of 'access_level' is used for runner registration or not. The option 'access_level' has been ignored on registration so far and was only used on updates. The user is informed by a deprecation warning, if the option is unspecified. For reasons of compatibility 'false' is assumed in that case. The option 'access_level_on_creation' will switch to 'true' for the next major release (community.general 7.0.0) Signed-off-by: Christoph Fiehe <[email protected]> Co-authored-by: Christoph Fiehe <[email protected]> (cherry picked from commit 31ff3f6)

felixfontein · 2023-01-30T20:04:59Z

@cfiehe thanks a lot for your contribution!
@nejch thanks a lot for reviewing this!

Creating a follow-up issue or PR sounds good to me. The first step will be removing the current default and documenting it manually, and using a similar mechanism as for the new option in this PR (if the default is not set, call module.deprecate(...) and explicitly set it to the old default). The deprecation period for this default needs to be longer though, it should be for c.g 8.0.0.

…not idempotent on first run after runner creation (#5922) Fixes #5907: gitlab_runner is not idempotent on first run after runner creation (#5908) This fix introduces the new boolean option 'access_level_on_creation'. It controls, whether the value of 'access_level' is used for runner registration or not. The option 'access_level' has been ignored on registration so far and was only used on updates. The user is informed by a deprecation warning, if the option is unspecified. For reasons of compatibility 'false' is assumed in that case. The option 'access_level_on_creation' will switch to 'true' for the next major release (community.general 7.0.0) Signed-off-by: Christoph Fiehe <[email protected]> Co-authored-by: Christoph Fiehe <[email protected]> (cherry picked from commit 31ff3f6) Co-authored-by: cfiehe <[email protected]>

ansibullbot added bug This issue/PR relates to a bug gitlab module module plugins plugin (any type) source_control labels Jan 28, 2023

nejch reviewed Jan 28, 2023

View reviewed changes

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch 2 times, most recently from 5f8fe38 to 448a583 Compare January 28, 2023 13:57

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Jan 28, 2023

felixfontein reviewed Jan 28, 2023

View reviewed changes

changelogs/fragments/5907-fix-gitlab_runner-not-idempotent.yml Outdated Show resolved Hide resolved

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch 3 times, most recently from 00b36d2 to 82d899d Compare January 29, 2023 16:53

felixfontein reviewed Jan 29, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 29, 2023

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch from 4849dd5 to 6f50b3d Compare January 29, 2023 17:08

ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 29, 2023

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch 2 times, most recently from d61c655 to a238592 Compare January 29, 2023 17:33

felixfontein reviewed Jan 29, 2023

View reviewed changes

plugins/modules/gitlab_runner.py Show resolved Hide resolved

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch from a238592 to f3bfb21 Compare January 29, 2023 20:41

cfiehe force-pushed the fix_gitlab_runner_not_idempotent branch from f3bfb21 to 4b12d51 Compare January 30, 2023 19:02

nejch approved these changes Jan 30, 2023

View reviewed changes

felixfontein merged commit 31ff3f6 into ansible-collections:main Jan 30, 2023

patchback bot mentioned this pull request Jan 30, 2023

[PR #5908/31ff3f66 backport][stable-6] Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5922

Merged

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 30, 2023

nejch mentioned this pull request Jan 30, 2023

Align gitlab_runner access_level default with GitLab #5925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5908

Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5908

cfiehe commented Jan 28, 2023 •

edited

Loading

ansibullbot commented Jan 28, 2023

nejch Jan 28, 2023 •

edited

Loading

nejch Jan 28, 2023

cfiehe Jan 28, 2023

nejch Jan 28, 2023

felixfontein Jan 28, 2023

nejch Jan 28, 2023

felixfontein Jan 28, 2023 •

edited

Loading

github-actions bot commented Jan 28, 2023 •

edited

Loading

This comment was marked as outdated.

cfiehe commented Jan 29, 2023

felixfontein left a comment

nejch left a comment

patchback bot commented Jan 30, 2023 •

edited

Loading

felixfontein commented Jan 30, 2023

Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5908

Fixes #5907: gitlab_runner is not idempotent on first run after runner creation #5908

Conversation

cfiehe commented Jan 28, 2023 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Jan 28, 2023

nejch Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

nejch Jan 28, 2023

Choose a reason for hiding this comment

cfiehe Jan 28, 2023

Choose a reason for hiding this comment

nejch Jan 28, 2023

Choose a reason for hiding this comment

felixfontein Jan 28, 2023

Choose a reason for hiding this comment

nejch Jan 28, 2023

Choose a reason for hiding this comment

felixfontein Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2023 • edited Loading

Docs Build 📝

This comment was marked as outdated.

cfiehe commented Jan 29, 2023

felixfontein left a comment

Choose a reason for hiding this comment

nejch left a comment

Choose a reason for hiding this comment

patchback bot commented Jan 30, 2023 • edited Loading

Backport to stable-6: 💚 backport PR created

felixfontein commented Jan 30, 2023

cfiehe commented Jan 28, 2023 •

edited

Loading

nejch Jan 28, 2023 •

edited

Loading

felixfontein Jan 28, 2023 •

edited

Loading

github-actions bot commented Jan 28, 2023 •

edited

Loading

patchback bot commented Jan 30, 2023 •

edited

Loading