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

Change DependabotSecretsSelectedRepoIDs to []string #2401

Merged
merged 3 commits into from Aug 13, 2022

Conversation

gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Jun 28, 2022

Fixes: #2400.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewis gmlewis added Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging. labels Jun 28, 2022
@gmlewis
Copy link
Collaborator Author

gmlewis commented Jun 28, 2022

@lneves75 - could you please review and approve if you agree to this change when you have a chance?

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #2401 (23b6ed6) into master (fd22ee9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2401   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         119      119           
  Lines       10546    10546           
=======================================
  Hits        10342    10342           
  Misses        140      140           
  Partials       64       64           
Impacted Files Coverage Δ
github/actions_secrets.go 100.00% <ø> (ø)
github/dependabot_secrets.go 100.00% <100.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 fd22ee9...23b6ed6. Read the comment docs.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jun 28, 2022

Awaiting LGTM+Approval from two contributors to this repo before merging.

@lneves75
Copy link

@gmlewis apparently the problem lies on github's API implementation for both endpoints.
If we call the endpoint /orgs/myorg/actions/secrets/BAR with a set of strings we get a 422

{
	"message": "Invalid request.\n\nFor 'items', \"123\" is not an integer.\nFor 'items', \"456\" is not an integer.",
	"documentation_url": "https://docs.github.com/rest/reference/actions#create-or-update-an-organization-secret"
}

So I'm afraid that we need to have two distinct types 😞

type ActionSecretsSelectedRepoIDs []int64
type DependabotSecretsSelectedRepoIDs []string

WDYT ?

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jun 28, 2022

OK, that is really weird.
Can you please contact GitHub tech support and verify this correct?

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewis gmlewis changed the title Change SelectedRepoIDs to []string Change DependabotSecretsSelectedRepoIDs to []string Jun 28, 2022
@gmlewis
Copy link
Collaborator Author

gmlewis commented Jun 28, 2022

@lneves75 - I've updated the PR, if you could please try it out for correctness, that would be greatly appreciated!

@timrogers
Copy link
Contributor

timrogers commented Jul 2, 2022

OK, that is really weird. Can you please contact GitHub tech support and verify this correct?

👋🏻 Hey there! I'm the PM for the API at GitHub, and I happened to just spot this thread.

I can confirm that the current behaviour of the API is that:

Sorry for the pain! This is obviously a bit weird at the moment and is something we need to clean up - but in a way that minimises breaking changes for users.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jul 2, 2022

👋🏻 Hey there! I'm the PM for the API at GitHub, and I happened to just spot this thread.

Hi @timrogers - it's great to have you here! Thank you to you and your team for an amazing product... it is greatly appreciated!

I can confirm that the current behaviour of the API is that:

If I'm reading this correctly, then making them both use an array of strings should work.
However, when we tried this here: e3028fb
it failed... see comment from @lneves75 above: #2401 (comment)

So then we tried splitting it into two separate implementations, as you see this PR now.

Sorry for the pain! This is obviously a bit weird at the moment and is something we need to clean up - but in a way that minimises breaking changes for users.

No problem... we would just like to get it working, so if you have recommendations on how to proceed, that would be great.
We don't mind updating the repo when/if it changes... we are updating this repo all the time. 😄

@timrogers
Copy link
Contributor

@gmlewis So sorry - you’re right, I showed too much confidence and I actually misread the internal PR 😅: the Actions API really does only accept integers, whereas before it claimed incorrectly in the docs to accept strings.

Looking at the inputs, integers seem like the right data type, so I will raise an issue with the Dependabot team to look at making this more consistent in a non-breaking way.

Very glad I came by this repo to discover this and see all of the great work you’re doing to make the API accessible to Go developers ❤️

@timrogers
Copy link
Contributor

In other words, it looks like you’re doing exactly what you need to do in this PR!

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jul 2, 2022

Very glad I came by this repo to discover this and see all of the great work you’re doing to make the API accessible to Go developers

Thank you so much, @timrogers !

We really appreciate your input, and we always love to hear from GitHub team members and tech support whenever we can. 😄

Copy link
Contributor

@raynigon raynigon left a comment

Choose a reason for hiding this comment

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

LGTM

@gmlewis
Copy link
Collaborator Author

gmlewis commented Aug 13, 2022

Thank you, @raynigon !
Merging.

@gmlewis gmlewis merged commit ce1f6b9 into google:master Aug 13, 2022
@gmlewis gmlewis deleted the i2400-selected-repo-ids branch August 13, 2022 22:20
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependabot Org Secrets creation returns 422 error
4 participants