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

Additonal revert of #2877 #2886

Merged
merged 2 commits into from
Jun 28, 2022
Merged

Additonal revert of #2877 #2886

merged 2 commits into from
Jun 28, 2022

Conversation

tkoyama010
Copy link
Member

@tkoyama010 tkoyama010 commented Jun 27, 2022

Overview

Additonal revert of #2877

Details

@tkoyama010 tkoyama010 marked this pull request as ready for review June 27, 2022 22:22
@tkoyama010 tkoyama010 enabled auto-merge (squash) June 27, 2022 22:22
@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jun 27, 2022
@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #2886 (6ff5b37) into main (8ba5eca) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2886   +/-   ##
=======================================
  Coverage   94.04%   94.04%           
=======================================
  Files          76       76           
  Lines       16428    16428           
=======================================
  Hits        15449    15449           
  Misses        979      979           

akaszynski
akaszynski previously approved these changes Jun 27, 2022
banesullivan
banesullivan previously approved these changes Jun 27, 2022
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

Still LGTM.

@akaszynski
Copy link
Member

@tkoyama010, since I've check and we indeed have the right permissions in Workflow permissions
, should we make sure this doesn't run on forks?

@tkoyama010
Copy link
Member Author

@akaszynski No. Thanks and sorry for this trouble. I will try a dummy account to see if it works.

@adeak
Copy link
Member

adeak commented Jun 27, 2022

@akaszynski No. Thanks and sorry for this trouble. I will try a dummy account to see if it works.

If my PRs are any indication, it's enough to open a PR from a branch on tkoyama010/pyvista instead of pyvista/pyvista.

@tkoyama010
Copy link
Member Author

tkoyama010 commented Jun 27, 2022

@adeak Thanks. I forked it now https://github.com/tkoyama010/pyvista . Could you please open test Pull Request with "document" label? This is after merged this Pull Request.

@adeak
Copy link
Member

adeak commented Jun 27, 2022

Sure, I can do that, just ping me once this is merged (I'll probably forget otherwise).

But we should make sure we all mean the same thing. I took @akaszynski's comment about "running on forks" to refer to cases like #2885: someone opening a PR to pyvista/pyvista from a branch at user/pyvista. So what I meant was for you to do that: opening a proper pyvista PR, but instead of using a (test) feature branch on the pyvista/pyvista repo, use a branch on your fork (like I did with my fork at adeak/pyvista).

From what you said it seems you suggested that I open a PR to your fork from my fork. And now I'm not sure what @akaszynski meant exactly 😄 I can help with any configuration, assuming we three agree on a test setup.

@tkoyama010 tkoyama010 merged commit 6bdedb7 into main Jun 28, 2022
@tkoyama010 tkoyama010 deleted the maint/revert branch June 28, 2022 00:21
@tkoyama010
Copy link
Member Author

@adeak Now I made a forked repository.

@adeak
Copy link
Member

adeak commented Jun 28, 2022

@tkoyama010 I think @akaszynski tested this already in #2889.

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

Successfully merging this pull request may close these issues.

None yet

4 participants