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

Add Project-level MR approvals #356

Merged

Conversation

husunal
Copy link

@husunal husunal commented Jul 3, 2020

This MR adds a Project-level MR approvals resource.

@husunal
Copy link
Author

husunal commented Jul 3, 2020

Any idea why TestAccGitlabServiceJira_basic started to fail?

##[error] TestAccGitlabServiceJira_basic: testing.go:684: Step 1 error: Check failed: Check 2/8 error: gitlab_service_jira.jira: Attribute 'url' expected "https://testurl.com", got "https://test.com"?
https://github.com/terraform-providers/terraform-provider-gitlab/pull/356/checks?check_run_id=834966420#step:5:168

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

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

Looks great! A few comments.

@husunal husunal force-pushed the feature/gitlab_project_approval_configuration branch 2 times, most recently from 239af25 to 1c08bc2 Compare July 21, 2020 15:06
@husunal
Copy link
Author

husunal commented Jul 21, 2020

Can we run acceptance-ee tests on pull_requests? When we use isRunningInCE in SkipFunc we don't run ee only tests in actions.

@horjulf
Copy link

horjulf commented Jul 29, 2020

Shouldn't this be part of the gitlab_project resource ?
There are already some MR check options in that resource (e.g. approvals_before_merge), this seems to be unique per project, so would make sense to keep it all in one place.

@husunal
Copy link
Author

husunal commented Jul 29, 2020

@horjulf totally makes sense but the API doesn't support it https://docs.gitlab.com/ee/api/projects.html#create-project

@horjulf
Copy link

horjulf commented Jul 29, 2020

@horjulf totally makes sense but the API doesn't support it https://docs.gitlab.com/ee/api/projects.html#create-project

I see, that does make it tricky to manage in a single resource 🤔

@jeremad
Copy link
Contributor

jeremad commented Aug 7, 2020

Do you need any help with this one? I'd really like to see this released. I can have a look at your failing test!

@husunal
Copy link
Author

husunal commented Aug 7, 2020

@jeremad feel free to contribute. I’ ve tested the module and the tests on a Gitlab EE but it fails in Github actions despite the SkipFunc which should be skipping tests for community edition and run for EE only.

@jeremad
Copy link
Contributor

jeremad commented Aug 7, 2020

I think it is expected that the tests run but they should be skipped immediately and only take a few ms (at least that's what happens with GitHub tests on master: https://github.com/terraform-providers/terraform-provider-gitlab/runs/936184044?check_suite_focus=true)

@armsnyder
Copy link
Collaborator

You also need to add your new doc page to website/gitlab.erb.

@jeremad
Copy link
Contributor

jeremad commented Aug 10, 2020

I fixed the review comments, squashed the commits and rebased on master I can open another MR, or you can push force that here:
https://github.com/jeremad/terraform-provider-gitlab/tree/feature/gitlab_project_approval_configuration

Copy link
Collaborator

@armsnyder armsnyder left a comment

Choose a reason for hiding this comment

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

LGTM. The only remaining thing would be changing project_id type to TypeInt, but that's minor.

@husunal
Copy link
Author

husunal commented Aug 17, 2020

Do we need additional approvals before merge?

Adding @nicholasklick

@armsnyder
Copy link
Collaborator

Looks like this needs rebasing @husunal

@husunal husunal force-pushed the feature/gitlab_project_approval_configuration branch from 4be2970 to a8ad4ad Compare September 7, 2020 20:29
@husunal
Copy link
Author

husunal commented Sep 7, 2020

Thanks, rebased @nicholasklick @armsnyder

@armsnyder
Copy link
Collaborator

Thanks! This looks ready to merge @nicholasklick

@armsnyder armsnyder merged commit 0ff40d5 into gitlabhq:master Sep 10, 2020
armsnyder added a commit to armsnyder/terraform-provider-gitlab that referenced this pull request Sep 10, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants