Add support for Dependabot secrets #2248

liath · 2022-01-11T01:39:09Z

I added handling for Dependabot specific secrets per #2246. This part of the API seems undocumented, but I don't see why it would change as it's almost identical to managing regular secrets. This file is pretty repetitive, and I doubled the number of repo and org methods, so I did some refactoring. Hopefully nothing too heinous.

gmlewis

Thank you, @liath !
Please add unit tests for the new methods.

github/actions_secrets.go

gmlewis · 2022-01-11T03:20:33Z

github/actions_secrets.go

+	return s.getPublicKey(ctx, url)
+}
+
+// GetRepoDependabotPublicKey gets a public key that should be used for secret encryption.


Maybe a more specific description for this endpoint would be good?

github/actions_secrets.go

codecov · 2022-01-11T03:35:53Z

Codecov Report

Merging #2248 (3dc35e6) into master (6e66a89) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2248      +/-   ##
==========================================
+ Coverage   97.80%   97.81%   +0.01%     
==========================================
  Files         113      114       +1     
  Lines       10207    10265      +58     
==========================================
+ Hits         9983    10041      +58     
  Misses        156      156              
  Partials       68       68

Impacted Files	Coverage Δ
github/actions_secrets.go	`100.00% <100.00%> (ø)`
github/dependabot_secrets.go	`100.00% <100.00%> (ø)`
github/github.go	`97.62% <100.00%> (+<0.01%)`	⬆️
github/users_packages.go	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e66a89...3dc35e6. Read the comment docs.

jurre · 2022-01-11T08:52:37Z

Slightly bike-sheddy, but I would suggest pulling this out of github/actions_secrets.go into github/dependabot_secrets.go. While the API shares a lot of similarities, it is it's own top-level resource that is geared towards Dependabot specifically, not Actions.

gmlewis

This is looking great, @liath - thank you!
Just a couple minor tweaks and then I think we will be ready for a second LGTM/Approval before merging.

Note that any other contributor to this repo can review and give the second LGTM/Approval. Thank you!

github/dependabot_secrets.go

github/dependabot_secrets_test.go

gmlewis

Oh, one more thing... since the new methods are part of the ActionsService, can you please rename the new files to actions_dependabot_secrets.go and actions_dependabot_secrets_test.go? Thank you!

…vice and update CC dates

liath · 2022-01-12T04:08:16Z

Done! That feels much nicer, good suggestion :D

liath · 2022-01-12T04:11:06Z

Could we go a step further and make them actions_secrets_dependabot* so that all the secrets-management files are next to each other alphabetically?

gmlewis · 2022-01-12T12:21:37Z

Could we go a step further and make them actions_secrets_dependabot* so that all the secrets-management files are next to each other alphabetically?

Genius! Sorry I didn't think of that the first time, @liath !

jurre · 2022-01-12T12:47:54Z

Not super familiar with this project, but I do work on Dependabot (and implemented the API in question) and so I'm curious why this is part of the ActionsService, it seems more appropriate to add a new DependabotService struct, as I'd expect these APIs might diverge over time which could result in a breaking change to the library. Given that they are separate resources in the API, would it make sense for the client to follow that?

No strong opinion either way, confident y'all do what makes most sense for this project 👍

gmlewis · 2022-01-12T12:57:43Z

Not super familiar with this project, but I do work on Dependabot (and implemented the API in question) and so I'm curious why this is part of the ActionsService, it seems more appropriate to add a new DependabotService struct, as I'd expect these APIs might diverge over time which could result in a breaking change to the library. Given that they are separate resources in the API, would it make sense for the client to follow that?

No strong opinion either way, confident y'all do what makes most sense for this project +1

Hi @jurre and thank you for the comments and questions (and for the API! 😄 )!

So in this repo, we try to tightly couple the layout and services to the GitHub v3 API documentation organization structure.

We initially placed these API endpoints under the ActionsService endpoints primarily because they appear to have a very high correlation with those endpoints, and because no Dependabot documentation was available at that time (if I'm not mistaken).

However, now that the API documentation exists and is indeed under a completely different section, I think you are right that we should create a new DependabotService.

@liath - would you like to tackle that, or should we address that in a follow-up PR (either by you or by another contributor)?

jurre · 2022-01-12T13:36:10Z

very high correlation with those endpoints

You're right, that's definitely the case 👍 they share a lot of the same semantics, but might evolve separately

no Dependabot documentation was available at that time

Yeah it took a few days to get those published so that's totally fair.

liath · 2022-01-12T19:02:12Z

Should I add Dependabot specific types like DependabotSecrets to fully separate them or would that create weirdness for callers?

gmlewis · 2022-01-12T19:32:49Z

I think we can keep the shared data structs for now (even though we are separating out the service)... with the understanding that if they diverge in the future, we can split it further.

gmlewis · 2022-01-12T19:34:16Z

And just to be clear, I think we can keep all that nice work you did to consolidate the shared code... again with the understanding that it may change some day, but I think it is super-clean for now, which is very nice.

gmlewis

Hey, this looks fantastic, @liath ! Thank you!
LGTM.
Awaiting second LGTM.

Maybe @Parker77 has time to review?

Parker77

LGTM

gmlewis · 2022-01-13T03:16:31Z

Thank you, @Parker77 !
Merging.

Add support for Dependabot secrets

ee5fb9c

gmlewis requested changes Jan 11, 2022

View reviewed changes

Move Dependabot secrets handling to its own file and add unit tests

0e65faf

liath mentioned this pull request Jan 11, 2022

Add support for Dependabot secrets integrations/terraform-provider-github#1006

Closed

liath requested a review from gmlewis January 11, 2022 19:36

gmlewis requested changes Jan 12, 2022

View reviewed changes

github/dependabot_secrets.go Outdated Show resolved Hide resolved

github/dependabot_secrets_test.go Outdated Show resolved Hide resolved

gmlewis requested a review from wesleimp January 12, 2022 03:37

gmlewis requested changes Jan 12, 2022

View reviewed changes

Rename dependabot secrets to indicate they're part of the Actions ser…

ebac560

…vice and update CC dates

liath added 2 commits January 12, 2022 10:57

Make a DependabotService and move secrets management into it

80c52f3

Revert changes to Actions secrets

094bc33

Revert the last revert, got a little carried away :P

3dc35e6

liath requested a review from gmlewis January 13, 2022 00:59

gmlewis approved these changes Jan 13, 2022

View reviewed changes

Parker77 approved these changes Jan 13, 2022

View reviewed changes

gmlewis merged commit 758d208 into google:master Jan 13, 2022

gmlewis mentioned this pull request Jan 13, 2022

Bump version of go-github library to v42 #2253

Merged

liath deleted the dependabot-secrets branch January 13, 2022 19:45

liath mentioned this pull request Jan 13, 2022

Add Dependabot secrets support (Fixes #1006) integrations/terraform-provider-github#1036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Dependabot secrets #2248

Add support for Dependabot secrets #2248

liath commented Jan 11, 2022

gmlewis left a comment

gmlewis Jan 11, 2022

codecov bot commented Jan 11, 2022 •

edited

jurre commented Jan 11, 2022

gmlewis left a comment

gmlewis left a comment

liath commented Jan 12, 2022

liath commented Jan 12, 2022

gmlewis commented Jan 12, 2022

jurre commented Jan 12, 2022 •

edited

gmlewis commented Jan 12, 2022

jurre commented Jan 12, 2022

liath commented Jan 12, 2022

gmlewis commented Jan 12, 2022

gmlewis commented Jan 12, 2022

gmlewis left a comment

Parker77 left a comment

gmlewis commented Jan 13, 2022

Add support for Dependabot secrets #2248

Add support for Dependabot secrets #2248

Conversation

liath commented Jan 11, 2022

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis Jan 11, 2022

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2022 • edited

Codecov Report

jurre commented Jan 11, 2022

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

liath commented Jan 12, 2022

liath commented Jan 12, 2022

gmlewis commented Jan 12, 2022

jurre commented Jan 12, 2022 • edited

gmlewis commented Jan 12, 2022

jurre commented Jan 12, 2022

liath commented Jan 12, 2022

gmlewis commented Jan 12, 2022

gmlewis commented Jan 12, 2022

gmlewis left a comment

Choose a reason for hiding this comment

Parker77 left a comment

Choose a reason for hiding this comment

gmlewis commented Jan 13, 2022

codecov bot commented Jan 11, 2022 •

edited

jurre commented Jan 12, 2022 •

edited