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

[Feature Request] github milestone resource and data source #470

Merged
merged 4 commits into from
Nov 23, 2020
Merged

[Feature Request] github milestone resource and data source #470

merged 4 commits into from
Nov 23, 2020

Conversation

anGie44
Copy link
Contributor

@anGie44 anGie44 commented May 24, 2020

Closes #295

Changes include:

  • new resource/data-source: github_repository_milestone
  • import support with: owner/repository/number

Output of acceptance tests:

--- PASS: TestAccGithubRepositoryMilestone_disappears (13.65s)
--- PASS: TestAccGithubRepositoryMilestone_update (17.49s)
--- PASS: TestAccGithubRepositoryMilestone_basic (17.65s)
--- PASS: TestAccGithubRepositoryMilestone_multiple (21.82s)

--- PASS: TestAccGithubRepositoryMilestoneDataSource_noMatchReturnsError (0.34s)
--- PASS: TestAccGithubRepositoryMilestoneDataSource_existing (8.96s)

@ghost ghost added size/XL Type: Documentation Improvements or additions to documentation labels May 24, 2020
@anGie44 anGie44 changed the title add milestone resource and data source [Feature Request] github milestone resource and data source May 24, 2020
jcudit
jcudit approved these changes May 24, 2020
View reviewed changes
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Couple of suggestions but nothing to block on.

github/data_source_github_repository_milestone.go
return err
}

d.SetId(strconv.FormatInt(milestone.GetID(), 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on new resources using the global node IDs when SetId() is being called?

Previous experimentation in https://github.com/terraform-providers/terraform-provider-github/pull/460 takes this approach:

https://github.com/terraform-providers/terraform-provider-github/blob/23c6faefa7da8039b9015e4c85b154a7928f685b/github/resource_github_project_card.go#L67

https://github.com/terraform-providers/terraform-provider-github/blob/23c6faefa7da8039b9015e4c85b154a7928f685b/github/resource_github_project_card_test.go#L63

github/resource_github_repository_milestone.go

func resourceGithubRepositoryMilestoneCreate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*Organization).v3client
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

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

Recently came across this:

https://github.com/terraform-providers/terraform-provider-github/blob/518829b4f87f8613feaa86b4046702a962619830/github/provider.go#L136

Does the provider become more responsive to a cancel operation if we wire this context all the way through to requests like this?

@jcudit jcudit modified the milestones: v2.10.0, v2.9.0 Jun 3, 2020
@jcudit jcudit modified the milestones: v2.10.0, v3.1.0 Jul 10, 2020
@anGie44 anGie44 modified the milestones: v3.1.0, v3.2.0 Jul 31, 2020
@ewilde
Copy link
Contributor

ewilde commented Nov 13, 2020

@jcudit is there anything left to do on this PR. I'm happy to help out if it's missing anything 👍

@jcudit jcudit force-pushed the ap_milestones_support branch from 518829b to 41446df Compare November 22, 2020 20:27
anGie44 and others added 4 commits November 23, 2020 09:46
@jcudit jcudit force-pushed the ap_milestones_support branch from 6c0afde to 94c06b9 Compare November 23, 2020 14:46
@jcudit
Copy link
Contributor

jcudit commented Nov 23, 2020

@ewilde thanks for the ping on this one 😄

This has been rebased and the tests have been brought in line with the latest suite. Look out for an upcoming CHANGELOG PR and raise any issues once this releases. Happy to work with any data you can bring back on the next bugfix release if this doesn't play well with your use case.

@jcudit jcudit merged commit 35b1a36 into master Nov 23, 2020
@jcudit jcudit deleted the ap_milestones_support branch November 23, 2020 14:49
@ewilde
Copy link
Contributor

ewilde commented Nov 23, 2020

@jcudit fantastic, thank you for taking the time to get this feature into the provider 👍 It looks like it will be a good match for my basic use case

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
@anGie44
[Feature Request] github milestone resource and data source (integrat…
988b3ec 
…ions#470)

* add milestone resource and data source

* fix misspell

* rebase and fix build errors

- update to go-github v32
- use `Owner` instead of `Organization`

* update test suite

Co-authored-by: Jeremy Udit <jcudit@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcudit jcudit jcudit approved these changes

Assignees
No one assigned
Labels
size/XL Type: Documentation Improvements or additions to documentation
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.

[Feature] Add milestone support
3 participants
@anGie44 @ewilde @jcudit