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.

Sign up for GitHub

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

Jump to bottom

Implement gitlab service slack #96

Merged
merged 8 commits into from
May 1, 2019
Merged

Implement gitlab service slack #96

merged 8 commits into from
May 1, 2019

Conversation

jorcau
Copy link
Collaborator

@jorcau jorcau commented Mar 4, 2019

Hi,

Here is a new proposal for Slack Integration implementation. It reuses work started on #65. I've talked with @hypnoglow about doing another PR so I guess we could close #65 for this one.

It includes documentation and acceptance tests:

➜ make testacc TEST=./gitlab TESTARGS='-run=TestAccGitlabServiceSlack_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./gitlab -v -run=TestAccGitlabServiceSlack_* -timeout 120m
=== RUN   TestAccGitlabServiceSlack_basic
--- PASS: TestAccGitlabServiceSlack_basic (8.23s)
=== RUN   TestAccGitlabServiceSlack_import
--- PASS: TestAccGitlabServiceSlack_import (6.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-gitlab/gitlab	15.209s

Thanks,

@roidelapluie
Copy link
Collaborator

FYI now the Acc tests are run in travis too.

@roidelapluie
Copy link
Collaborator

roidelapluie commented Mar 4, 2019 •
edited
Loading

Can you please add an ACC test with default values? Current tests are great but too explicit. I would like another, minimalist one.

@roidelapluie roidelapluie mentioned this pull request Mar 4, 2019
Closed
@jorcau jorcau force-pushed the implement-gitlab-service-slack branch from 56d51a5 to c3e4c15 Compare March 4, 2019 13:43
@jorcau
Copy link
Collaborator Author

jorcau commented Mar 4, 2019 •
edited
Loading

FYI now the Acc tests are run in travis too.

This is awesome!

I added a test with only required fields set as you asked.

@jorcau jorcau force-pushed the implement-gitlab-service-slack branch from c3e4c15 to 0295fa0 Compare March 5, 2019 17:31
roidelapluie
roidelapluie reviewed Mar 5, 2019
View reviewed changes
gitlab/resource_gitlab_service_slack.go

func resourceGitlabServiceSlackImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
d.Set("project", d.Id())

Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldnt we fetch the service at this point from gitlab?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you mean here.

From what I understand, the resourceGitlabServiceSlackImportState function should just define the ID or necessary parameters so the READ function can then be called to actually fetch the resource's details.

@jorcau jorcau force-pushed the implement-gitlab-service-slack branch from 0295fa0 to 6dfd2cb Compare March 8, 2019 15:36
@rsrchboy
Copy link

rsrchboy commented Apr 3, 2019

@roidelapluie Are there any blockers to merging this?

@jorcau
Copy link
Collaborator Author

jorcau commented Apr 30, 2019

Hi @roidelapluie, do you have time to review & merge this anytime soon? Thanks!

@roidelapluie roidelapluie merged commit 959dabd into gitlabhq:master May 1, 2019
@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.
Reviewers

@roidelapluie roidelapluie roidelapluie left review comments

Assignees
No one assigned
Labels
documentation size/XL
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jorcau @roidelapluie @rsrchboy @hypnoglow