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

Feature/gitlab deploykey updkey #1661

Merged

Conversation

morco
Copy link
Contributor

@morco morco commented Jan 22, 2021

SUMMARY

This PR makes it possible to update the public key itself of a gitlab project
deploy key (identified by the key title). It can be used for regularly updating
the secret while keeping some kind of identity by reusing the same key title.
We use it together with HashiCorp Vault to short cycle secrets.
Note that this is not directly supported by the gitlab API. We solve this
internally by deleting the old key object first if one exists and than
recreate it with the new public key and the old name.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

gitlab_deploy_key

ADDITIONAL INFORMATION

Be aware that technically this is a breaking change for edge cases as if beforehand
a different public key value was given as parameter to an existing key title, I'm
pretty sure it simply did nothing while with this PR it obviously actually updates the key
in the server.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added affects_2.10 community_review feature This issue/PR relates to a feature request gitlab integration tests/integration module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) source_control tests tests labels Jan 22, 2021
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Jan 22, 2021
@ansibullbot ansibullbot added community_review and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 22, 2021
@morco morco marked this pull request as ready for review January 22, 2021 19:23
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need a changelog fragment.

I think it makes sense to have two sections in the fragment, one with minor_changes (declaring this a feature) and one with breaking_changes (warning that the behavior changes). Since the next release is 2.0.0 (next week), changing behavior is accepted, and I think this is a case where a deprecation / extra option to enable/disable this isn't necessary.

@Lunik @Shaps @dj-wasabi @marwatk @scodeman @waheedi @zanssa dear maintainers, what do you think?

@@ -145,6 +145,13 @@ def __init__(self, module, gitlab_instance):
def createOrUpdateDeployKey(self, project, key_title, key_key, options):
changed = False

# note: unfortunately public key cannot be updated directly by
# gitlap rest api, so for that case we need to delete and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# gitlap rest api, so for that case we need to delete and
# GitLab REST API, so for that case we need to delete and

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added the changelog fragment and fixed the comment. Let me know if I should do anything else.

@felixfontein
Copy link
Collaborator

Since this is a breaking change, it either has to be merged until in < two days (for community.general 2.0.0), or in ~6 months (for community.genreal 3.0.0).

@Lunik @Shaps @dj-wasabi @marwatk @scodeman @waheedi @zanssa dear maintainers, please take a look! This is urgent.

@zanssa
Copy link
Contributor

zanssa commented Jan 28, 2021

Can we review the parameters in the test integration file? I think login_token should be replaced with api_token, and server_url should be replaced with api_url!

@felixfontein. will take a look during the weekend :)

@felixfontein
Copy link
Collaborator

@zanssa it uses the same module options in the new integration test entry as in the other integration test entries in the same file. Well, and these are indeed wrong; I guess the integration tests have been broken and wrong for a long time now... The module and the tests didn't change (except maybe details) since they were added to this repo on the initial commit (from the ansible/ansible repo).

I'd say we'd ignore the integration tests now. We have until in ~5 hours to decide whether to merge this now, or wait for community.general 3.0.0.

@zanssa
Copy link
Contributor

zanssa commented Feb 1, 2021

@felixfontein, okay. Module looks good to me!

@morco, thanks for the contribution.

@felixfontein felixfontein added the breaking_change This PR contains a breaking change that MUST NOT be backported label Feb 5, 2021
@felixfontein felixfontein merged commit dd0b54b into ansible-collections:main Feb 5, 2021
@felixfontein
Copy link
Collaborator

@morco thanks for implementing this!
@zanssa thanks for reviewing this!

I've marked the PR as 'breaking change' and did not backport it to stable-2, that means it has to wait for community.general 3.0.0.

@morco morco deleted the feature/gitlab-deploykey-updkey branch February 9, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change This PR contains a breaking change that MUST NOT be backported community_review feature This issue/PR relates to a feature request gitlab integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) source_control tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants