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

Add client methods to support Actions artifact APIs. #1480

Merged
merged 5 commits into from
Sep 14, 2022

Conversation

lerebear
Copy link
Contributor

@lerebear lerebear commented Sep 8, 2022

Partially addresses #1216.

This adds support for the Actions artifact APIs documented at https://docs.github.com/en/rest/actions/artifacts.

Help needed please 🙏🏿

  • As a first-time contributor, I need a maintainer to approve my pending Actions workflows on this PR please.
  • It isn't clear to me how to record a new cassette for these tests. I've tried hardcoding the default record_mode to :once and running tests with either script/test or bundle exec rspec spec/octokit/client/actions_artifacts_spec.rb, both to no avail. The lack of cassettes for these new tests will likely cause them to fail once the pending Actions workflows are authorized, so any guidance here would be much appreciated.
  • I'm not sure how to satisfy the documentation requirement from the contribution guide. I've run bundle exec rake doc:yard locally, but I'm not sure how to interpret its output. Please let me know if there's anything more to do here.

@lerebear lerebear changed the title Draft support for downloading actions artifacts Add support for Actions artifact APIs. Sep 13, 2022
@lerebear lerebear changed the title Add support for Actions artifact APIs. Add client methods to support Actions artifact APIs. Sep 13, 2022
@lerebear lerebear force-pushed the action-artifacts branch 2 times, most recently from 72e9a46 to 9b8a8d1 Compare September 13, 2022 04:14
@lerebear lerebear marked this pull request as ready for review September 13, 2022 04:54
Copy link
Contributor

@timrogers timrogers left a comment

Choose a reason for hiding this comment

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

This looks great to me from a code perspective ✨ I don't think there is anything you need to worry about from a documentation perspective as this all looks good to me. I'll make a note to revisit CONTRIBUTING.md.

I'm not sure how to get VCR working - I've had similar issues. But @Brend-Smits managed to get round this with his recent PR, #1478, so I am wondering if he might be able to help?

@Brend-Smits
Copy link
Contributor

This looks great to me from a code perspective ✨ I don't think there is anything you need to worry about from a documentation perspective as this all looks good to me. I'll make a note to revisit CONTRIBUTING.md.

I'm not sure how to get VCR working - I've had similar issues. But @Brend-Smits managed to get round this with his recent PR, #1478, so I am wondering if he might be able to help?

VCR automatically recorded cassettes for me. It only does so once though.
You might need to set environment variables to enable record mode. See the helper method here: https://github.com/octokit/octokit.rb/blob/main/spec/helper.rb#L130-L148

- Remove Webmock stubs (these were causing requests to be identified by
VCR as externally stubbed, which causes them to be ignored during the
record and replay phases)
- Correct bugs revealed by making actual requests
@lerebear
Copy link
Contributor Author

It isn't clear to me how to record a new cassette for these tests.

I figured this out with a some help from @juanmrad 🙌🏿 (another recent contributor to this project).

In my case, I had misunderstood the interaction between Webmock and VCR. Specifically, my tests originally used Webmock stubs like so:

request = stub_get("repos/#{@test_repo}/actions/runs/#{@workflow_run_id}/artifacts")

When VCR encounters that request, it recognizes it as "externally stubbed" and ignores it.

Once I removed those stubs, then VCR began attempting to record cassettes for new requests as expected. From there, I did indeed have to set the environment variables described in the docs on running and writing new tests that were relevant to my tests. Those were OCTOKIT_TEST_GITHUB_LOGIN, OCTOKIT_TEST_GITHUB_TOKEN and OCTOKIT_TEST_GITHUB_REPOSITORY. To avoid needing to set unnecessary environment variables, I took two additional steps:

  1. I ran just my newly added test suite with bundle exec rspec spec/octokit/client/actions_artifacts_spec.rb rather than script/test.
  2. I commented out the following portion of the test setup, which assumes that the tests need a real organization-owned repository (which mine do not):

    octokit.rb/spec/helper.rb

    Lines 118 to 123 in 15be725

    test_org_repo = "#{test_github_org}/#{test_github_repository}"
    unless oauth_client.repository?(test_org_repo, options)
    Octokit.octokit_warn "NOTICE: Creating #{test_org_repo} test repository."
    options[:organization] = test_github_org
    oauth_client.create_repository(test_github_repository, options)
    end

With those changes in place I was able to record and replay cassettes as expected.

One last note is that the VCR debug logging config was pretty helpful in making the above discoveries; I enabled that along the way too.

@lerebear lerebear requested a review from timrogers September 14, 2022 00:53
@lerebear
Copy link
Contributor Author

@timrogers would you mind providing another review of this please? I think my pending workflows need to be approved again too.

I also wonder if you can explain the remainder of the release process? Specifically I'm curious as to:

  • Whether or not I should merge this pull request myself once its approved
  • When this feature might be released in a new version of this library
  • If features like this are ever backported to older versions of Octokit (specifically 4.x)

Thanks!

@timrogers
Copy link
Contributor

@timrogers would you mind providing another review of this please? I think my pending workflows need to be approved again too.

Sure!

I also wonder if you can explain the remainder of the release process? Specifically I'm curious as to:

  • Whether or not I should merge this pull request myself once its approved
  • When this feature might be released in a new version of this library
  • If features like this are ever backported to older versions of Octokit (specifically 4.x)

Of course:

  1. One of the maintainers will take care of that once a PR has been approved.
  2. We generally release a new version as soon as new features are merged.
  3. We have never done that historically, but we could do if there was clear value to it.

# @param repo [Integer, String, Repository, Hash] A GitHub repository
#
# @return [Sawyer::Resource] the total count and an array of artifacts
# @see https://developer.github.com/v3/actions/artifacts#list-artifacts-for-a-repository
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# @see https://developer.github.com/v3/actions/artifacts#list-artifacts-for-a-repository
# @see https://docs.github.com/en/rest/actions/artifacts#list-artifacts-for-a-repository

lib/octokit/client/actions_artifacts.rb Outdated Show resolved Hide resolved
lib/octokit/client/actions_artifacts.rb Outdated Show resolved Hide resolved
lib/octokit/client/actions_artifacts.rb Outdated Show resolved Hide resolved
lib/octokit/client/actions_artifacts.rb Outdated Show resolved Hide resolved
@timrogers timrogers merged commit e5abb57 into octokit:main Sep 14, 2022
@timrogers
Copy link
Contributor

Released to RubyGems in v5.6.0.

@timrogers
Copy link
Contributor

It isn't clear to me how to record a new cassette for these tests.

I figured this out with a some help from @juanmrad 🙌🏿 (another recent contributor to this project).

In my case, I had misunderstood the interaction between Webmock and VCR. Specifically, my tests originally used Webmock stubs like so:

request = stub_get("repos/#{@test_repo}/actions/runs/#{@workflow_run_id}/artifacts")

When VCR encounters that request, it recognizes it as "externally stubbed" and ignores it.

Once I removed those stubs, then VCR began attempting to record cassettes for new requests as expected. From there, I did indeed have to set the environment variables described in the docs on running and writing new tests that were relevant to my tests. Those were OCTOKIT_TEST_GITHUB_LOGIN, OCTOKIT_TEST_GITHUB_TOKEN and OCTOKIT_TEST_GITHUB_REPOSITORY. To avoid needing to set unnecessary environment variables, I took two additional steps:

  1. I ran just my newly added test suite with bundle exec rspec spec/octokit/client/actions_artifacts_spec.rb rather than script/test.
  2. I commented out the following portion of the test setup, which assumes that the tests need a real organization-owned repository (which mine do not):

    octokit.rb/spec/helper.rb

    Lines 118 to 123 in 15be725

    test_org_repo = "#{test_github_org}/#{test_github_repository}"
    unless oauth_client.repository?(test_org_repo, options)
    Octokit.octokit_warn "NOTICE: Creating #{test_org_repo} test repository."
    options[:organization] = test_github_org
    oauth_client.create_repository(test_github_repository, options)
    end

With those changes in place I was able to record and replay cassettes as expected.

One last note is that the VCR debug logging config was pretty helpful in making the above discoveries; I enabled that along the way too.

I'd very much welcome a contribution to the docs on this ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants