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

Correct error when providing a string for http_proxy #116

Closed
wants to merge 1 commit into from

Conversation

tfardet
Copy link
Collaborator

@tfardet tfardet commented Jan 18, 2023

np.isnan does not support str entries so the test used to trigger an error whenever an actual value was provided

np.isnan does not support str entries so the test used to trigger an error whenever an actual value was provided
@tfardet
Copy link
Collaborator Author

tfardet commented Jan 19, 2023

Don't know if I'm supposed to do something for the tests to pass... clearly some credentials are missing but I guess it should be configured to use your secrets for PRs, right?
It runs fine on my local machine with my credentials.

@hadrilec
Copy link
Contributor

Indeed I have to check how to allow external PRs to have access to repo's secrets, if you find any guideline on this let me know, in any case I will check how to do it in the coming days

@hadrilec
Copy link
Contributor

I have just added you as an active contributor to this repo, let me know if it goes in the right direction

@tfardet
Copy link
Collaborator Author

tfardet commented Jan 19, 2023

Thanks, I'll try to have a look (hopefully this evening or tomorrow), I'm not sure that write access alone will change the problem, though, probably there'll be something more to do.

@tfardet
Copy link
Collaborator Author

tfardet commented Jan 20, 2023

OK, I looked around and I think pull_request_target is the way to go!
See here for a discussion about the actual code that should be used:

name: pynsee package tests

on:
  push:
      branches:
        - 'master'
  pull_request_target:
    branches:
        - 'master'

jobs:
  build:
    runs-on: ubuntu-latest
    strategy:
      matrix:
        python-version: ["3.7", "3.8", "3.9", "3.10"]

    steps:
      - name: Checkout code
        uses: actions/checkout@v2
        with:
          ref: "refs/pull/${{ github.event.number }}/merge"

and this page for a discussion about how to make it safe.

@hadrilec
Copy link
Contributor

hadrilec commented Feb 7, 2023

@tfardet thanks a lot, I modified the package test yaml file following your guidelines.
I also included your bug fix in this commit d508b12
If you are working behind a proxy server, I would be happy if you could let me know if pynsee is still usable behind the proxy

@tfardet
Copy link
Collaborator Author

tfardet commented Feb 8, 2023

OK, good, closing this, then, will try to check it out ASAP and work on fixing the tests (I've already seen some strings that need to be rawstrings)

@tfardet tfardet closed this Feb 8, 2023
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.

2 participants