Disable integration tests for PRs coming from forks about nodejs HOT 8 CLOSED

Sounds a bit fishy as well. What does this mean?

Third-party service needs to implement a new authentication method on the server side so that the JWT token is recognized and verified.

Also, this means that everyone contributing from a fork, has to change .travis.yml in order to provide the custom credentials.
Additionally, this will require that all external contributors have to do this even though they didn't provide any integration test, otherwise the build won't run.
I still think that disabling is the better solution for now.

from nodejs.

That's unfortunately not easy to do, and not reliable. Also, bootstrapping / deleting projects takes more time than the actual integration test itself :/

But that's already a discussion that has been going on for a long time.

from nodejs.

Sorry wrong button 🙈

from nodejs.

So any external PR doesn't get the results of integration tests? That sounds like a really bad idea to me or am I missing something?

from nodejs.

Exactly. I mean this is a fundamental problem with a build system (like travis) that has encrypted variables and it's a big security issue.

https://docs.travis-ci.com/user/pull-requests#Pull-Requests-and-Security-Restrictions

I read some discussions about this and most of the time people just recommend to disable such tasks from an external PR. I also read that for example saucelabs was thinking of having a complex "authentication" mechanism with like public/private keys to allow such cases.

But I don't see much of a problem to be honest:

• integration tests are decoupled from the packages, so providing unit tests when contributing to packages still allows PRs to be built correctly
• external contributors can still provide integration tests and run them locally with their own credentials
• when we merge, integration tests will run again, so we will see anyway if something broke

from nodejs.

Yeah it seems like a tough 🥜 too crack.

when we merge, integration tests will run again, so we will see anyway if something broke

That sounds a bit late. What about this solution?

from nodejs.

I had a quick chat with @PhilippSpo. The jwt approach is not good.

Ideally we should just get from the API a way of getting a temporary token (e.g. valid for 10min) for one of the test projects. No client credentials needed anymore.
More ideally, the API should provide a pool of test projects to use for integration tests, which can be cleaned from the backend.

Until then, I think this is the best way to go ahead with it. But we should bring up this conversation with the backend team.

from nodejs.

Well ideally we can create and delete projects. Because afaik that's how they do tests on the backend team. :)

from nodejs.