Closed amitmawkin closed 7 years ago
Need to check why travis is failing tests when they are succeeding locally
@amitmawkin thanks for sending us this PR. 👍 we will try to take a look at it tomorrow (unless it's urgent for your work tonight...?)
@nelsonic thanks no urgency as i can always pack my local packages with the changes, I will spend some time tomorrow to check why travis tests are failing and local succeeds thanks again
@amitmawkin so... the reason Travis-CI is failing is simple: the PR is coming from outside our Org. (so travis blocks the encrypted environment variables from being accessed for security reasons) if you push your code to a branch within @dwyl we can help you debug the Travis-CI build. (BTW: based on the fact that you have added good tests for your contribution and prepared a well-documented Pull Request closely following the contributing guide, you have been granted write access to the repo: https://github.com/dwyl/sendemail/invitations and an Org invite: https://github.com/dwyl )
@nelsonic this should be in good shape now, it is failing due to lack of .env file . Also as .env file contains AWS profile , tokens etc and we are not suppose to check it in so not sure how to test it. Please review.
@amitmawkin the code looks good. 😍
I have granted you write access to dwyl. https://github.com/dwyl/sendemail/invitations
Please push your branch to
this
origin: git@github.com:dwyl/sendemail.git
and re-create the PR with the branch in the dwyl the org.
this will allow us to debut the CI build with you. 👍
@amitmawkin we ❤️ your enthusiasm and really appreciate you wanting to contribute code! 🎉
but ... dwyl/contributing > #3-pr-ettiquete bullet point 3 ... #NeverMergeYourOwnPullRequest
...
(our apologies if this is unclear from the request above...)
@iteles we need to proliferate the CONTRIBUTING.md
through all @dwyl repos.
(it's not @amitmawkin's "fault" that the PR contribution steps ("workflow") were not clear...
but this cannot happen again... we use this package in Production for several projects and cannot afford for any code to be merged in without rigorous review.) 😞
Thankfully, merging into master
does not publish a new version to NPM, so there's no "crisis"...
But it's still lame to have to "explain" that we need "secondary approval" for all PRs otherwise we get into "trouble" with our Customers who required us to follow ISO27001 A.12.1.2 (change management control) 😢
@nelsonic my mistake, i was trying to push the branch upstream but ended up pushing to master, as soon as I realized it I reverted it back, I have pushed the branch now for build and review. Sorry for the confusion.
ENV file format can be as below for newer fields