Implement support for actions artifacts #1414

sprsld · 2020-02-09T00:40:35Z

Support for the actions artifacts plus tests.

Relates to #1399 .

googlebot · 2020-02-09T00:40:44Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

gmlewis

Thank you, @Atorr. This is a good start but needs more work. Please see below.

gmlewis · 2020-02-09T01:17:40Z

github/actions_artifacts.go

+	Name               *string `json:"name,omitempty"`
+	SizeInBytes        *int64  `json:"size_in_bytes,omitempty"`
+	ArchiveDownloadURL *string `json:"archive_download_url,omitempty"`
+	Expired            *bool   `json:"expired,omitempty"`


Can you please contact GitHub tech support at support@github.com and ask if they are actually sending the expired field as a string or as a JSON boolean? Their docs show a string, but I would think they should be a boolean.
If they say these are indeed strings like the example shows, we may need to change this field to a *string instead of a *bool. You can report their reply back here, please.

Sure thing.

gmlewis · 2020-02-09T01:17:58Z

github/actions_artifacts.go

+	SizeInBytes        *int64  `json:"size_in_bytes,omitempty"`
+	ArchiveDownloadURL *string `json:"archive_download_url,omitempty"`
+	Expired            *bool   `json:"expired,omitempty"`
+	CreatedAt          *string `json:"created_at,omitempty"`


This and the next field need to be of type *Timestamp, please.

gmlewis · 2020-02-09T01:19:17Z

github/actions_artifacts.go

+// ListWorkflowRunArtifacts lists all artifacts that belong to a workflow run.
+//
+// GitHub API docs: https://developer.github.com/v3/actions/artifacts/#list-workflow-run-artifacts
+func (s *ActionsService) ListWorkflowRunArtifacts(ctx context.Context, owner, repo string, runID int64, opt *ListOptions) (*ArtifactList, *Response, error) {


Can you please change opt to opts? I know the repo is inconsistent in this regard, and it is probably mostly my fault, but I would like to standardize on opts because I think it makes more sense (as the type is ListOptions).
So while I'm maintaining the repo, I figure I can ask for what I prefer. 😄 Thanks.
Maybe I should file an issue to change all "opt" to "opts" to make it consistent, then I don't have to ramble on and on like this. 😄 OK, done: #1415.

#1417 is already merged, so please change this from opt to opts. Thank you!

gmlewis · 2020-02-09T01:34:33Z

github/actions_artifacts.go

+// DownloadArtifact gets a redirect URL to download an archive for a repository.
+//
+// GitHub API docs: https://developer.github.com/v3/actions/artifacts/#download-an-artifact
+func (s *ActionsService) DownloadArtifact(ctx context.Context, owner, repo string, artifactID int64, archiveType string) (*Response, error) {


First, I was going to ask archiveType to be renamed to archiveFormat to match the GitHub documentation.
But then I realized that this value must be "zip" and thought about hard-coding it.
However, since they made it a parameter, let's go ahead and rename the variable... and add a comment that its value currently must be "zip".

Thoughts?

I could actually go either way on this one since we have semver we could hard-code the value until it changes later, then bump the version.

Ah, now I see below on line 87 that you reject the call if the value is not "zip"... I'm heavily leaning toward removing the parameter and hard-coding the value in the URL path on line 85.

I agree with hard-coding it. Will make the change.

Not sure where I got archiveType, as I had intended to keep the naming the same as the documentation. 😅

gmlewis · 2020-02-09T01:41:56Z

github/actions_artifacts.go

+		return nil, err
+	}
+
+	location := String("")


No, I don't think this is what we want. We want to either return the URL or follow the redirect URL and return the actual ZIP file.

I think this whole function needs to be modeled after RepositoriesService.GetArchiveLink located here:
https://github.com/google/go-github/blob/master/github/repos_contents.go#L242-L288

Whoops I had meant to get rid of that. ~~Will have it return the URL.~~ Will model it after RepositoriesService.GetArchiveLink.

gmlewis · 2020-02-09T01:45:59Z

github/actions_artifacts_test.go

+	client, _, _, teardown := setup()
+	defer teardown()
+
+	_, _, err := client.Actions.GetArtifact(context.Background(), "%", "r", 1)


What is this actually testing? I think it can be deleted.

I saw other test files have cases to sanity check parameters (for example). This is just to make sure whatever is given for either the owner or repo can be URL-parsed. Looks like I forgot to switch the parameter for some of the test functions, thus why they look like duplicates. Given that, do you still prefer to have these tests removed?

Given that, do you still prefer to have these tests removed?

I'm totally fine to keep the tests if they are not duplicates and they are testing something. 😄
Thanks!

Sure thing.

OK, I'm not actually sure what changed here. Are all the tests actually testing something?

Yes. These are sanity checks for the owner and repo parameters in each function. They should not longer be duplicates as they were all updated in my latest commit.

Here is a link to each test case in the file view:
ListWorkflowRunArtifacts_invalidOwner
ListWorkflowRunArtifacts_invalidRepo
GetArtifact_invalidOwner
GetArtifact_invalidRepo
DownloadArtifact_invalidOwner
DownloadArtifact_invalidRepo
DeleteArtifact_invalidOwner
DeleteArtifact_invalidRepo

gmlewis · 2020-02-09T01:46:12Z

github/actions_artifacts_test.go

+	client, _, _, teardown := setup()
+	defer teardown()
+
+	_, err := client.Actions.DownloadArtifact(context.Background(), "%", "r", 1, "zip")


What is this actually testing? I think it can be deleted.

#1414 (comment)

gmlewis · 2020-02-09T01:46:18Z

github/actions_artifacts_test.go

+	client, _, _, teardown := setup()
+	defer teardown()
+
+	_, err := client.Actions.DownloadArtifact(context.Background(), "%", "r", 1, "zip")


What is this actually testing? I think it can be deleted.

#1414 (comment)

gmlewis · 2020-02-09T01:46:39Z

github/actions_artifacts_test.go

+	client, _, _, teardown := setup()
+	defer teardown()
+
+	_, err := client.Actions.DeleteArtifact(context.Background(), "%", "r", 1)


What is this actually testing? I think it can be deleted.

#1414 (comment)

gmlewis · 2020-02-09T01:46:45Z

github/actions_artifacts_test.go

+	client, _, _, teardown := setup()
+	defer teardown()
+
+	_, err := client.Actions.DeleteArtifact(context.Background(), "%", "r", 1)


What is this actually testing? I think it can be deleted.

#1414 (comment)

gmlewis · 2020-02-10T01:05:30Z

github/actions_artifacts_test.go

+		)
+	})
+
+	opt := &ListOptions{Page: 2}


When you get a chance, could you please change opt to opts to be consistent with #1417?
(Here and anywhere else you find it in this PR, please.)

gmlewis · 2020-02-10T01:05:40Z

github/actions_secrets.go

+// without revealing their encrypted values.
+//
+// GitHub API docs: https://developer.github.com/v3/actions/secrets/#list-secrets-for-a-repository
+func (s *ActionsService) ListSecrets(ctx context.Context, owner, repo string, opt *ListOptions) (*Secrets, *Response, error) {


When you get a chance, could you please change opt to opts to be consistent with #1417?
(Here and anywhere else you find it in this PR, please.)

gmlewis · 2020-02-10T01:05:50Z

github/actions_secrets_test.go

+		fmt.Fprint(w, `{"total_count":4,"secrets":[{"name":"A","created_at":"2019-01-02T15:04:05Z","updated_at":"2020-01-02T15:04:05Z"},{"name":"B","created_at":"2019-01-02T15:04:05Z","updated_at":"2020-01-02T15:04:05Z"}]}`)
+	})
+
+	opt := &ListOptions{Page: 2, PerPage: 2}


When you get a chance, could you please change opt to opts to be consistent with #1417?
(Here and anywhere else you find it in this PR, please.)

…ue-1399/actions-artifacts

martinssipenko · 2020-02-11T07:19:10Z

@googlebot I consent.

googlebot · 2020-02-11T07:19:19Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gmlewis

Thank you, @Atorr!
LGTM.

Awaiting second LGTM before merging.

sprsld · 2020-02-12T02:21:10Z

Thanks @gmlewis! I am still waiting to hear back from Github support regarding the Artifact expired field. I have not gotten any word back from them as of yet. Should we wait for their reply to merge?

gmlewis · 2020-02-12T02:30:20Z

Should we wait for their reply to merge?

Ah, right... I had forgotten about this. Can you actually test out your implementation and see if a string is truly being sent like the documentation shows or if an unquoted JSON bool (true/false) is being sent?

Actually, if a string ("true"/"false") is being sent, but your code as-written (which uses a *bool) works, then I think we don't need to wait for their reply, but could just update the (possibly closed) issue when we hear from them for posterity.

Thanks, @Atorr!

sprsld · 2020-02-12T07:24:04Z

It looks as though the expired field is a JSON bool as shown in the following example

curl -v -L -u <user>:<PAT> "https://api.github.com/repos/atorr/python-test/actions/runs/38008073/artifacts"
*   Trying 192.30.255.117...
* TCP_NODELAY set
* Connected to api.github.com (192.30.255.117) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: /etc/ssl/certs
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Unknown (8):
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Client hello (1):
* TLSv1.3 (OUT), TLS Unknown, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN, server accepted to use http/1.1
* Server certificate:
*  subject: C=US; ST=California; L=San Francisco; O=GitHub, Inc.; CN=*.gh.neting.cc
*  start date: Jul  8 00:00:00 2019 GMT
*  expire date: Jul 16 12:00:00 2020 GMT
*  subjectAltName: host "api.github.com" matched cert's "*.gh.neting.cc"
*  issuer: C=US; O=DigiCert Inc; OU=www.digicert.com; CN=DigiCert SHA2 High Assurance Server CA
*  SSL certificate verify ok.
* Server auth using Basic with user 'atorr'
* TLSv1.3 (OUT), TLS Unknown, Unknown (23):
> GET /repos/atorr/python-test/actions/runs/38008073/artifacts HTTP/1.1
> Host: api.github.com
> Authorization: Basic YXRvcnI6ODlhNjg0YWU2YzZlYTM3ZDMyMjI1ZDQ4ZTA3ZDIzMWFhZGYyZmRlMQ==
> User-Agent: curl/7.58.0
> Accept: */*
>
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS Unknown, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.3 (IN), TLS Unknown, Unknown (23):
< HTTP/1.1 200 OK
< Date: Wed, 12 Feb 2020 07:20:39 GMT
< Content-Type: application/json; charset=utf-8
< Content-Length: 484
< Server: GitHub.com
< Status: 200 OK
< X-RateLimit-Limit: 5000
< X-RateLimit-Remaining: 4968
< X-RateLimit-Reset: 1581494537
< Cache-Control: private, max-age=60, s-maxage=60
< Vary: Accept, Authorization, Cookie, X-GitHub-OTP
< ETag: "c2af0ecd706568f833f5ef6246cfe0db"
< X-OAuth-Scopes: workflow
< X-Accepted-OAuth-Scopes:
< X-GitHub-Media-Type: github.v3; format=json
< Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type
< Access-Control-Allow-Origin: *
< Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
< X-Frame-Options: deny
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
< Content-Security-Policy: default-src 'none'
< Vary: Accept-Encoding, Accept
< X-GitHub-Request-Id: EF27:5FE3:3E2FA:4F608:5E43A747
<
{
  "total_count": 1,
  "artifacts": [
    {
      "id": 1781805,
      "node_id": "MDg6QXJ0aWZhY3QxNzgxODA1",
      "name": "README",
      "size_in_bytes": 14,
      "url": "https://api.github.com/repos/atorr/python-test/actions/artifacts/1781805",
* TLSv1.3 (IN), TLS Unknown, Unknown (23):
      "archive_download_url": "https://api.github.com/repos/atorr/python-test/actions/artifacts/1781805/zip",
      "expired": false,
      "created_at": "2020-02-12T07:01:24Z",
      "updated_at": "2020-02-12T07:01:53Z"
    }
  ]
}
* Connection #0 to host api.github.com left intact

gmlewis · 2020-02-12T13:56:05Z

Excellent! Full steam ahead then... GitHub can fix their documented examples when they get around to it and we can continue on.

gmlewis · 2020-02-22T11:48:20Z

Friendly ping for a second LGTM - @gauntface or @wesleimp.

gmlewis · 2020-02-27T02:27:04Z

@wesleimp or @martinssipenko - if you approve this, we could merge it. Thanks.

martinssipenko

LGTM

gmlewis · 2020-02-27T12:41:20Z

Thank you, @martinssipenko!
Merging.

Relates to: google#1399.

martinssipenko and others added 9 commits January 28, 2020 16:04

Implement support for action secrets

5ed8db9

Added missing delete test

1908e4b

Implement PR feedback

00f2f03

Added info on how to encrypt secrets

bbff48b

Implement 2nd round of feedback

6367ea6

fix my broken english

bbe3d53

feature: implement support for actions artifacts

c244d23

style: add missing period to comment

ed7ef40

googlebot added the cla: no label Feb 9, 2020

gmlewis mentioned this pull request Feb 9, 2020

Change "opt" param names to "opts" everywhere to make the repo consistent #1415

Closed

sprsld requested a review from gmlewis February 9, 2020 01:43

gmlewis requested changes Feb 9, 2020

View reviewed changes

gmlewis mentioned this pull request Feb 9, 2020

Support GitHub Actions API #1399

Closed

gmlewis reviewed Feb 10, 2020

View reviewed changes

sprsld added 2 commits February 10, 2020 22:57

Merge branch 'master' of https://github.com/google/go-github into iss…

69eeb3a

…ue-1399/actions-artifacts

fix: make changes request in PR review

3aa2f08

googlebot added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Feb 11, 2020

gmlewis approved these changes Feb 12, 2020

View reviewed changes

gmlewis requested a review from gauntface February 12, 2020 02:17

martinssipenko approved these changes Feb 27, 2020

View reviewed changes

gmlewis merged commit 6563560 into google:master Feb 27, 2020

sprsld deleted the issue-1399/actions-artifacts branch March 11, 2020 20:50

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Implement support for actions artifacts (google#1414)

aa849e1

Relates to: google#1399.

Implement support for actions artifacts #1414

Implement support for actions artifacts #1414

Conversation

sprsld commented Feb 9, 2020

googlebot commented Feb 9, 2020

gmlewis 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

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

sprsld Feb 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprsld Feb 12, 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

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 commented Feb 11, 2020

googlebot commented Feb 11, 2020

gmlewis left a comment

Choose a reason for hiding this comment

sprsld commented Feb 12, 2020

gmlewis commented Feb 12, 2020

sprsld commented Feb 12, 2020

gmlewis commented Feb 12, 2020

gmlewis commented Feb 22, 2020

gmlewis commented Feb 27, 2020

martinssipenko left a comment

Choose a reason for hiding this comment

gmlewis commented Feb 27, 2020

sprsld Feb 9, 2020 •

edited

Loading

sprsld Feb 12, 2020 •

edited

Loading