-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(e2e): migrate off circleci #4576
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
76a7298
to
d765026
Compare
Signed-off-by: Rui Chen <[email protected]> update deps Signed-off-by: Rui Chen <[email protected]> update ngrok to v3 add NGROK_AUTH_TOKEN Signed-off-by: Rui Chen <[email protected]>
d765026
to
a7e83ef
Compare
Signed-off-by: Rui Chen <[email protected]>
326e638
to
c02c95d
Compare
Signed-off-by: Rui Chen <[email protected]>
@@ -20,14 +15,15 @@ cd "${CIRCLE_WORKING_DIRECTORY}/e2e" | |||
sleep 2 | |||
|
|||
# start ngrok in the background and wait for it to start | |||
./ngrok config add-authtoken $NGROK_AUTH_TOKEN | |||
./ngrok http 4141 > /tmp/ngrok.log & | |||
./ngrok config add-authtoken $NGROK_AUTH_TOKEN > /dev/null 2>&1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any cases where this might fail and having the stderr output would be useful for debugging, rather than being suppressed?
If we captured the combined output; and only print it if $?
is not 0
from it; we would get the best of both worlds.
Alternatively could this be moved into the Setup deps
step as part of the installation and keeping them together (letting GH deal with hiding the output by default when things are successful).
Moving it into the pipeline would also reduce NGROK_AUTH_TOKEN
env from a all steps has this
to Setup ngrok
step can access it; limiting possible exposure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same deal as version management, if it failed or does not work for any reason, we will probably debug directly inside the container though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just bring some echo statement back so that we know this step does not work (that is why I disabled all the outputs in here)
Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
Co-authored-by: Christian Winther <[email protected]>
Co-authored-by: Christian Winther <[email protected]>
Co-authored-by: Christian Winther <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
5b2f048
to
50ff2ff
Compare
should be good to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some of @jippi comments are pretty valid. Feel free to proceed but I want to make sure we address them in some form as long as it's non-blocking.
Going to include |
Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]> chore: add another patch for fix forked PR run issue Signed-off-by: Rui Chen <[email protected]>
#4607) Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]> Co-authored-by: Christian Winther <[email protected]>
migrate off circleci
previous efforts:
fixes #2660