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

gitlab: use HookActions map only if it is necessary #56

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

herbetom
Copy link
Contributor

@herbetom herbetom commented Jul 29, 2020

Since Gitlab 13.2 an approve and unapprove Action is available in Core: https://gitlab.com/help/user/project/merge_requests/merge_request_approvals#optional-approvals-core-only

The intent with this PR is to only use the Value form the HookAction if a value is found. Whether this is really what happens must be tested by someone. I have no environment in which I could easily test this.

@herbetom herbetom changed the title Add approve and unapprove as possible actions gitlab: Add approve and unapprove as possible actions Jul 29, 2020
@mweinelt
Copy link
Contributor

mweinelt commented Jul 29, 2020

Does it make sense to fallback to the raw value and not map values where in == out? This has the added benefit to be more accepting of new states.

@herbetom
Copy link
Contributor Author

Yes, that makes perfect sense. It's just more "complicated" to implement.

@herbetom herbetom changed the title gitlab: Add approve and unapprove as possible actions gitlab: use map of HookActions only if value found Aug 5, 2020
@herbetom
Copy link
Contributor Author

herbetom commented Aug 5, 2020

@fleaz Could this work?

@herbetom herbetom changed the title gitlab: use map of HookActions only if value found gitlab: use HookActions map only if it is necessary Aug 5, 2020
@fleaz
Copy link
Owner

fleaz commented Aug 10, 2020

Hey @herbetom,
sorry for the delay.

This looks good to me, and also thanks to @mweinelt to the requested change.

Can you change your PR to target the development branch, than I will merge this 👍

@herbetom herbetom changed the base branch from master to development August 11, 2020 00:49
@herbetom
Copy link
Contributor Author

I changed the target branch. We will see if it works 🤷‍♂️

@fleaz
Copy link
Owner

fleaz commented Aug 11, 2020

LGTM 👍

@fleaz fleaz merged commit 0cc6ecd into fleaz:development Aug 11, 2020
@herbetom herbetom deleted the addApproveHookAction branch October 24, 2020 14:18
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 this pull request may close these issues.

3 participants