Gh 271 GitHub secrets #362

benj-fletch · 2020-02-24T14:07:27Z

This PR covers functionality described in GH-271

The following functionality has been provided by this new code:

Data source github_actions_public_key - retrieving the public key of a repository
Data source github_actions_secrets - retrieval of all secrets in a repository
Resource github_actions_secret - management of Actions Secrets

See the documentation for details around config for each new item.

Note that this requires #342 to be merged prior to this and so it has been raised as a draft PR

martinssipenko · 2020-02-24T14:15:58Z

github/resource_github_actions_secret.go

+				Required:  true,
+				Sensitive: true,
+			},
+			"key_id": {


Perhaps it's better to not have these arguments and instead just fetch and use the public key on create?

It's a good shout. I initially provided these to allow users to determine the key / key id without the need for github_actions_secrets_public_key but I think just not including them as you suggest is better.

martinssipenko · 2020-02-25T14:46:03Z

github/resource_github_actions_secret.go

+		Delete: resourceGithubActionsSecretDelete,
+
+		Schema: map[string]*schema.Schema{
+			"owner": {


Why is the owner argument required? Given you use checkOrganization func, that checks that organization is configured in provider settings, you could get the value from provider config via meta. As an example, take a look at repository webhook code.

I was unaware that I could pull the organisation from the meta to be honest!
Thinking about this, would there be any case that you would want to manage a secret that wasn't part of the organisation in the provider? I think it might be possible in the current implementation but should not be encouraged - forcing users to have an individual provider per organisation seems more appropriate?
If you 👍 my assertions above I am happy to remove the owner argument

Yes, same ideology is used when managing other repository resources, webhooks is one example.

martinssipenko · 2020-02-25T15:08:39Z

github/resource_github_actions_secret.go

+		return err
+	}
+
+	d.SetId(fmt.Sprintf("%s/%s:%s", owner, repo, secretName))


If we assume that owner comes from provider configuration, then you can't have more than a single owner value and the owner part in Id becomes redundant. Have you considered using two part id consisting of repo name and secret name, and combining them using buildTwoPartID utility func, you can find example here?

I agree. I saw that function but wanted to use all 3 values. If we remove owner as per above I will implement this suggestion

martinssipenko · 2020-02-25T15:11:56Z

github/resource_github_actions_secret.go

+		d.SetId("")
+		return err
+	}
+


How come there are no d.Set() method calls? I think the secrets information could be stored in state. The UpdatedAt value from github.Secret could be used to determine if the value has changed in GitHub, for example, if UpdatedAt stored in state differs from value you got in GetSecret response, it means someone has updated the value outside of Terraform, and we probably want to sync it up (reapply value defined in Terraform).

I wasn't aware that the d.Set() functions were required to pass things to state. My understanding was that the existing d values passed in would get persisted.
I fully agree with the use of UpdatedAt - would you recommend saving the full state of the github.Secret ?

I think I would store the repo and name as par of Id, then store plaintext_value as it is and lastly store updated_at as computed getting the value from create/update operation.

Sounds like a solid plan - I will also include created_at since we have it available and might as well make it available

martinssipenko · 2020-02-25T15:13:28Z

github/resource_github_actions_secret.go

+	client := meta.(*Organization).client
+	ctx := context.WithValue(context.Background(), ctxId, d.Id())
+
+	orgName := d.Get("owner").(string)


Consider getting orgName from provider config via meta here.

martinssipenko · 2020-02-25T15:18:45Z

github/resource_github_actions_secret.go

+	ctx := context.WithValue(context.Background(), ctxId, d.Id())
+
+	orgName := d.Get("owner").(string)
+	repoName := d.Get("repository").(string)


Perhaps it's better to obtain repoName and secretName by extracting it from d.GetId() using parseTwoPartID utility func?

repoName, secretName, err := parseTwoPartID(d.Id(), "repository", "secret_name") if err != nil { return err }

martinssipenko · 2020-02-25T15:28:14Z

Can't really figure out from the code, are we always updating the secret on GitHub, even if it has not changed locally?

benj-fletch · 2020-02-26T08:35:49Z

I didn't think we are updating every time, though reading this review, I was unaware that the secret data wasn't being saved to state so maybe we are always updating. As discussed further up in the review, I think we should add to the state and this will resolve your concern

martinssipenko · 2020-02-26T08:38:29Z

@benj-fletch sweet, if you need any help let me know. I'm happy to review again after you have meade the changes discussed above.

benj-fletch · 2020-02-26T10:09:42Z

Thanks for the comments @martinssipenko! I have updated the PR to hopefully address all of them. I'll leave it in draft still until the update to go github v29 has occured as I imagine it will create conflicts to resolve

martinssipenko · 2020-02-26T10:30:31Z

github/data_source_github_actions_public_key.go

+				Type:     schema.TypeString,
+				Required: true,
+			},
+			"owner": {


I think this too should come from provider settings via meta.

martinssipenko · 2020-02-26T10:34:02Z

github/data_source_github_actions_secrets.go

+	"log"
+)
+
+func dataSourceGithubActionsSecrets() *schema.Resource {


I'll be frank, I don't see any benefit of having this data source as the data you can obtain from secret is practically unusable.

I initially added this as I thought it might be useful in future resources / data sources for GitHub actions. Reviewing the available API endpoints I think I agree that this data is useless in the long run. I will remove this data source.

martinssipenko · 2020-02-26T10:39:11Z

github/resource_github_actions_secret.go

+			},
+			"secret_name": {
+				Type:     schema.TypeString,
+				Required: true,


Shall we add ForceNew: true here?

martinssipenko · 2020-02-26T10:57:18Z

github/resource_github_actions_secret.go

+	owner := meta.(*Organization).name
+	ctx := context.Background()
+
+	repo := d.Get("repository").(string)


The repo and secret name should be extracted from Id:

repoName, secretName, err := parseTwoPartID(d.Id(), "repository", "secret_name") if err != nil { return err }

Yes, I should have caught this in the previous PR comment resolution! Thanks.

martinssipenko · 2020-02-26T10:59:39Z

github/resource_github_actions_secret.go

+		return err
+	}
+
+	d.SetId(buildTwoPartID(&repo, &secretName))


The Id should not be set here, it should already exist by this point either coming from state or set by resourceGithubActionsSecretCreateOrUpdate func.

martinssipenko · 2020-02-26T11:00:36Z

github/resource_github_actions_secret.go

+	}
+
+	d.SetId(buildTwoPartID(&repo, &secretName))
+	d.Set("plaintext_value", d.Get("plaintext_value"))


Not 100% sure but I think is not required, as it already should exist in state.

I am also not sure on this point. I left it there such that it was obvious (and definite) it would be set. I don't see any issue with leaving this line in?

martinssipenko · 2020-02-26T11:01:58Z

github/resource_github_actions_secret.go

+	secretName := d.Get("secret_name").(string)
+
+	secret, _, err := client.Actions.GetSecret(ctx, owner, repo, secretName)
+	if err != nil || secret.Name != secretName {


Why is || secret.Name != secretName part needed here?

I don't think it is on review!

martinssipenko · 2020-02-26T11:02:35Z

github/resource_github_actions_secret.go

+	if err != nil {
+		return err
+	}
+


You should set the Id here using d.SetId(buildTwoPartID(&repo, &secretName)). Afterwards this func calls resourceGithubActionsSecretRead func, which instead should extract repo and secret name from the d.GetId() via parseTwoPartID func, I've given the example in this comment.

martinssipenko · 2020-02-27T04:25:41Z

Can you rebase from master? #342 was merged, but unfortunatley it includes v29.0.3, but we need v29.0.3 for this to work. The question is should we make update to latest in this or seperate PR?

benj-fletch · 2020-02-28T12:24:44Z

Hi @martinssipenko, thanks a lot for the re-review. I have rebased onto master and hopefully have addressed all your concerns, are you able to have another look?

N.B. This PR now relies on #369 for the go-github v29.0.3 upgrade and will need rebasing again to master following its merge.

btkostner · 2020-03-20T16:31:39Z

@benj-fletch #369 has been merged into master now. This can be rebased and hopefully merged! Thank you for all the work on this!

GH-271 - Added actions_public_key data resource GH-271 - Added actions_secrets data source Further testing is required on this data source GH-271 - Added actions_secret resource GH-271 - Adding actions_secret resource documentation updates Fix formatting Updates following PR comments Updates following PR comments GH-271 - Resolving more PR comments Updating documentation GH-271 - removing references to old TF SDK

benj-fletch · 2020-03-23T08:57:06Z

Thanks for the prod @btkostner - I have rebased, fixed a couple of conflicts and squashed the commits. This should be ready for review now! 💯

jcudit

This is looking great with no concerns from me on the first pass.

My next step is to validate acceptance tests are 👌 and forecast this merging by end of week.

jcudit · 2020-03-24T21:21:04Z

website/docs/r/actions_secret.html.markdown

+## Import
+
+Repositories can be imported using the `name`, e.g.
+
+```
+$ terraform import github_repository.terraform terraform
+```


This needs to be updated or removed

Good catch, classic copy paste error! Will update

jcudit · 2020-03-24T21:43:31Z

website/github.erb

@@ -13,6 +13,9 @@
        <li>
          <a href="#">Data Sources</a>
          <ul class="nav nav-visible">
+            <li>
+              <a href="/docs/providers/github/d/actions_public_key.html">github_actions_public_key</a>


I always miss this file, thanks.

jcudit

Acceptance tests are passing for me locally. 🚀

…github-secrets Gh 271 GitHub secrets

ghost added the size/XXL label Feb 24, 2020

benj-fletch mentioned this pull request Feb 24, 2020

Update go-github to v29. #342

Merged

martinssipenko reviewed Feb 24, 2020

View reviewed changes

martinssipenko reviewed Feb 25, 2020

View reviewed changes

martinssipenko suggested changes Feb 26, 2020

View reviewed changes

ghost added size/XL dependencies Type: Documentation Improvements or additions to documentation and removed size/XXL labels Feb 28, 2020

benj-fletch mentioned this pull request Feb 28, 2020

Upgrade to go-github version 29.0.3 #368

Closed

yordis mentioned this pull request Mar 5, 2020

Missing Features Aggregate #379

Closed

10 tasks

jakebiesinger-onduo and others added 3 commits March 23, 2020 08:46

Update go-github to v29.

c35b63f

Run go mod tidy to remove go-github v28 indirect dependency.

b99a72d

benj-fletch marked this pull request as ready for review March 23, 2020 08:55

jcudit reviewed Mar 24, 2020

View reviewed changes

Update website docs

a1a5f63

jcudit added the acceptance-test/TestAcc label Mar 25, 2020

jcudit approved these changes Mar 25, 2020

View reviewed changes

jcudit merged commit e880ec6 into integrations:master Mar 25, 2020

benj-fletch deleted the gh-271-github-secrets branch March 25, 2020 13:32

jcudit mentioned this pull request Mar 25, 2020

Fix Failing CI Tests #393

Merged

jcudit added this to the v2.5.0 milestone Mar 27, 2020

benj-fletch mentioned this pull request Apr 1, 2020

Feature request: Support secret creation for github actions #271

Closed

ejhayes mentioned this pull request May 18, 2020

[Feature Request] Organization Secrets #468

Closed

KMConner mentioned this pull request Feb 14, 2021

[Feature Request] Use encrypted_value in github_actions_organization_secret and github_actions_secret #704

Closed

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022

Merge pull request integrations#362 from dmwgroup/integrationsgh-271-…

c33ab7b

…github-secrets Gh 271 GitHub secrets

Gh 271 GitHub secrets #362

Gh 271 GitHub secrets #362

Conversation

benj-fletch commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benj-fletch Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinssipenko Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinssipenko commented Feb 25, 2020

benj-fletch commented Feb 26, 2020

martinssipenko commented Feb 26, 2020

benj-fletch commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinssipenko Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

martinssipenko commented Feb 27, 2020

benj-fletch commented Feb 28, 2020

btkostner commented Mar 20, 2020

benj-fletch commented Mar 23, 2020

jcudit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcudit left a comment

Choose a reason for hiding this comment

benj-fletch Feb 26, 2020 •

edited

Loading

martinssipenko Feb 25, 2020 •

edited

Loading

martinssipenko Feb 26, 2020 •

edited

Loading