Skip to content

Change DependabotSecretsSelectedRepoIDs to []string - #2401

Merged
gmlewis merged 3 commits into
google:masterfrom
gmlewis:i2400-selected-repo-ids
Aug 13, 2022
Merged

Change DependabotSecretsSelectedRepoIDs to []string#2401
gmlewis merged 3 commits into
google:masterfrom
gmlewis:i2400-selected-repo-ids

Conversation

@gmlewis

Copy link
Copy Markdown
Collaborator

Fixes: #2400.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewisgmlewis 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
Copy Markdown
CollaboratorAuthor

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

@codecov

codecovBot commented Jun 28, 2022

Copy link
Copy Markdown

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 FilesCoverage Δ
github/actions_secrets.go100.00% <ø> (ø)
github/dependabot_secrets.go100.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
Copy Markdown
CollaboratorAuthor

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

@lneves75

Copy link
Copy Markdown

@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 😞

typeActionSecretsSelectedRepoIDs []int64typeDependabotSecretsSelectedRepoIDs []string

WDYT ?

@gmlewis

Copy link
Copy Markdown
CollaboratorAuthor

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>
@gmlewisgmlewis changed the title Change SelectedRepoIDs to []stringChange DependabotSecretsSelectedRepoIDs to []stringJun 28, 2022
@gmlewis

Copy link
Copy Markdown
CollaboratorAuthor

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

@timrogers

timrogers commented Jul 2, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
CollaboratorAuthor

👋🏻 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
Copy Markdown
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
Copy Markdown
Contributor

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

@gmlewis

Copy link
Copy Markdown
CollaboratorAuthor

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. 😄

@raynigonraynigon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gmlewis

Copy link
Copy Markdown
CollaboratorAuthor

Thank you, @raynigon !
Merging.

@gmlewis
gmlewis merged commit ce1f6b9 into google:masterAug 13, 2022
@gmlewis
gmlewis deleted the i2400-selected-repo-ids branch August 13, 2022 22:20
@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 19, 2022
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API ChangePR 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

@gmlewis@lneves75@timrogers@raynigon