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.

Sign up for GitHub

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

Jump to bottom

Add StateReason to IssueUpdate #2665

Merged
merged 3 commits into from
Feb 21, 2023
Merged

Add StateReason to IssueUpdate #2665

merged 3 commits into from
Feb 21, 2023

Conversation

heaths
Copy link
Contributor

@heaths heaths commented Feb 1, 2023

Resolves #2664

Behavior

Before the change?

  • Could not set the state_reason when closing or reopening an issue. This negatively impacts task lists that link issues: can make you think an issue was actually completed vs. not planned.

After the change?

  • You can now set the state_reason when closing or reopening an issue.

Other information

  • I successfully ran the integration test I added. After an initial test run using the IntegrationTests target and a number of errors, I figured I don't have all the env vars set necessary but changed no other tests.

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • Adds argument to IssueUpdate constructor. But seems this has been done many times before. Would it be better to add an overload? I merely copied the pattern that's been used to date, it seems.

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@heaths
Copy link
Contributor Author

heaths commented Feb 1, 2023

If acceptable, I'd be happy to update Octokit for the other languages to add state_reason.

@nickfloyd
Copy link
Contributor

Hey @heaths, thanks for this change! ❤️ I'm taking a look at it now and will let you know! Feel free to make the change in octokit.rb if you're up for it. Octokit.js should already have it given the auto-generation that occurs.

@nickfloyd
Copy link
Contributor

Hey @heaths, just a quick note about the sourcelink failure in the windows CI build. It is unrelated to your change and is something we are working on. Apologies for the noise.

@nickfloyd nickfloyd mentioned this pull request Feb 6, 2023
Merged
5 tasks
@heaths
Copy link
Contributor Author

heaths commented Feb 6, 2023 •
edited

@nickfloyd I've almost got a change ready in octokit.rb, but on trying to re-record tests with VCR (Ruby 3.1.2) getting constant errors like:

     1.1) Failure/Error: super

          NoMethodError:
            undefined method `octokit_warn' for Octokit:Module
          # ./lib/octokit.rb:61:in `method_missing'
          # ./spec/helper.rb:114:in `block (2 levels) in <top (required)>'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/util/variable_args_block_caller.rb:9:in `call_block'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/util/hooks.rb:15:in `conditionally_invoke'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/util/hooks.rb:30:in `block in invoke_hook'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/util/hooks.rb:29:in `map'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/util/hooks.rb:29:in `invoke_hook'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/request_handler.rb:46:in `invoke_before_request_hook'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/request_handler.rb:8:in `handle'
          # /home/heaths/gems/gems/vcr-6.1.0/lib/vcr/library_hooks/webmock.rb:135:in `block in <module:WebMock>'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:33:in `block (2 levels) in register_global_stub'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:39:in `synchronize'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:39:in `block in register_global_stub'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/request_pattern.rb:40:in `matches?'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:73:in `block in request_stub_for'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:72:in `each'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:72:in `detect'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:72:in `request_stub_for'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/stub_registry.rb:64:in `response_for_request'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:74:in `request'
          # /home/heaths/gems/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:113:in `block in request_with_wrapped_block'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:114:in `start_without_connect'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:141:in `start'
          # /home/heaths/gems/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:112:in `request_with_wrapped_block'
          # /home/heaths/gems/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:102:in `perform_request'
          # /home/heaths/gems/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:66:in `block in call'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/adapter.rb:45:in `connection'
          # /home/heaths/gems/gems/faraday-net_http-3.0.2/lib/faraday/adapter/net_http.rb:65:in `call'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/middleware.rb:17:in `call'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/middleware.rb:17:in `call'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/middleware.rb:17:in `call'
          # ./lib/octokit/middleware/follow_redirects.rb:73:in `perform_with_redirection'
          # ./lib/octokit/middleware/follow_redirects.rb:61:in `call'
          # /home/heaths/gems/gems/faraday-retry-2.0.0/lib/faraday/retry/middleware.rb:148:in `call'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/rack_builder.rb:153:in `build_response'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/connection.rb:444:in `run_request'
          # /home/heaths/gems/gems/faraday-2.7.4/lib/faraday/connection.rb:280:in `post'
          # /home/heaths/gems/gems/sawyer-0.9.2/lib/sawyer/agent.rb:99:in `call'
          # ./lib/octokit/connection.rb:156:in `request'
          # ./lib/octokit/connection.rb:28:in `post'
          # ./lib/octokit/client/issues.rb:102:in `create_issue'
          # ./spec/octokit/client/issues_spec.rb:105:in `block (4 levels) in <top (required)>'
          # /home/heaths/gems/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

A cassette is generated (though pretty-printed - not compact like existing cassettes; not sure if that's a problem) but with only a single request/response pair - 201 Created. I've added 2 tests - keeping the others as regression tests - but updated one reopen_issue test since it should set state_reason to reopened. All of them fail to update in the same way.

I've granted my auth token every conceivable scope (would be nice if required scopes were documented), which has eliminated 401 Unauthorized errors initially. Looking through bugs I see no mention of this.

FWIW, I've been unable to rbenv install 2.7 or any 2.x I've tried because the openssl it pulls is explicitly blocked because of deprecated methods defined therein. And while I've dabbled in Ruby a bit, by no means would I say I'm comfortable with the overall ecosystem. The changes needed here are easy enough given the existing code.

Does your CI have any sort of "re-record for me" functionality?

Update: I was able to get Ruby 2.7.6 installed finally, but it made no difference. In fact, lots of errors about @owner not being set, but the generated cassettes ended up with the same problem: a single request/response pair that doesn't have the right content and fails assertions.

@nickfloyd
Copy link
Contributor

Update: I was able to get Ruby 2.7.6 installed finally, but it made no difference. In fact, lots of errors about @owner not being set, but the generated cassettes ended up with the same problem: a single request/response pair that doesn't have the right content and fails assertions.

Hey @heaths, oof, sorry for the trouble in Rubyland. The story there is less tidy than we'd like it to be. Please keep pressing if you still feel motivated to do it - we certainly appreciate your effort there. Things like compacting the cassettes aren't a top concern, and unfortunately, there is no replay built into CI (that would be an excellent idea for future efforts, though).

I'm going to work on getting what you did here merged in and released and will have to circle back around possibly this Friday to see about the ruby challenges.

Thank you again for all you're doing here! ❤️

@heaths
Copy link
Contributor Author

heaths commented Feb 7, 2023 •
edited

@nickfloyd if you want, I could push the changes I made for octokit.rb if someone else wouldn't mind taking a crack at re-recording. I'd love to know why it's not working, though. I can open an issue or start a discussion on that repo if that's preferred. I did see if anyone else was having that problem. If anything, maybe I can make some improvements to the docs to help others. For example, it'd be great to document which scopes your GitHub auth token needs. It took a little trial and error (and just some assumptions, like that it cleans up so I'll need delete_repo) to get right.

Updated: I started a discussion here.

@kfcampbell
Copy link
Member

@heaths are you all set with this PR and comfortable with it going in?

@heaths
Copy link
Contributor Author

heaths commented Feb 18, 2023

Yes, that would be great.

@heaths heaths mentioned this pull request Feb 21, 2023
Closed
1 task
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented Priority: High labels Feb 21, 2023
nickfloyd
nickfloyd approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

LGTM! ❤️ Thank you for the change!

@nickfloyd nickfloyd merged commit 8ba882e into octokit:main Feb 21, 2023
@heaths heaths deleted the issue2664 branch February 21, 2023 22:21
@heaths
Copy link
Contributor Author

heaths commented Feb 21, 2023 •
edited

Thank you, @nickfloyd. When might we see a new package published so we can take advantage of this?

/cc @jsquire @JimSuplizio

@heaths
Copy link
Contributor Author

heaths commented Mar 6, 2023

@nickfloyd @kfcampbell when might we expect the next package release so we can use these changes? TIA

@nickfloyd
Copy link
Contributor

Hey @heaths, apologies for the delay; we've been trying to catch up on some things. We plan to send this out the door this week, hopefully sooner rather than later...

Again sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickfloyd nickfloyd nickfloyd approved these changes

Assignees
No one assigned
Labels
Type: Bug Something isn't working as documented
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG]: state_reason is not exposed for issues, affects task lists
3 participants
@heaths @nickfloyd @kfcampbell