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

fix: resourcePath in authorizers should contain wildcards #980

Merged
merged 3 commits into from Jun 4, 2020

Conversation

danilofuchs
Copy link
Contributor

@danilofuchs danilofuchs commented May 18, 2020

Fixes #917

I still need to write tests for this fix.

I must say, the development experience to contribute to this project for the first time is quite poor.
The main issue is I could not run tests reliably in my machine:

  • every single time I want to run tests, npm installs every 'scenario' dependencies
  • I cannot run unit tests separately, only the whole project with many integration tests
  • I'm not sure where to add tests, considering unit tests are in a folder old-unit. I'm not confident in adding code to something with old in its name
  • Some random tests break by timeout
  • Husky runs tests on every push

In my opinion, tests should be easy to run locally, but not enforced to be ran on every push. Travis CI already does this tests for us, and is way more convenient. Running them locally on every push (which takes some minutes) will certainly make developers reconsider contributing ever again. For instance, Next.js is an immense project, and allows for flexible testing locally, and tests every PR on a CI.

If you would like, I can open an issue with my concerns so we can discuss it separately

@dherault
Copy link
Owner

Hi @danilofuchs thanks for your PR!

"Some random tests break by timeout" --> I know, it sucks, did not write those. I tend to forget about them.
"Husky runs tests on every push" --> Not anymore, ouf.

@danilofuchs danilofuchs marked this pull request as ready for review June 1, 2020 15:44
@chardos
Copy link
Collaborator

chardos commented Jun 2, 2020

Hi @danilofuchs, thanks for putting in this PR!

Would you be able to resolve the conflicts before we merge?

@danilofuchs
Copy link
Contributor Author

Would you be able to resolve the conflicts before we merge?

Sure! I just rebased my branch and added a simple integration test.

Thank you for taking my concerns into consideration. I now see it is much easier to test the repository

@danilofuchs danilofuchs changed the title fix: Resource path should contain wildcards fix: resourcePath in authorizers should contain wildcards Jun 3, 2020
@chardos chardos merged commit bf2a48d into dherault:master Jun 4, 2020
@danilofuchs danilofuchs deleted the fix/resource-path branch June 4, 2020 13:46
@zoellner zoellner mentioned this pull request Apr 13, 2022
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.

context.resourcePath does not match the one in API Gateway
3 participants