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
ci: use a named docker instance with proper working dir and env #1011
Conversation
Current coverage is 96.35% (diff: 100%)@@ master #1011 diff @@
==========================================
Files 194 194
Lines 17755 17755
Methods 0 0
Messages 0 0
Branches 1368 1368
==========================================
Hits 17107 17107
Misses 440 440
Partials 208 208
|
@3v1n0 Thanks. What were we missing without the TERM variable? Can you please report a bug for the problem that you are fixing? |
And export there the host's TERM variable.
d31408c
to
c43f1df
Compare
@ElOpio bug added.
This will ensure that the docker instance will inherit the TERM variable from the parent terminal, thus it will optionally hide all the unneeded content from logs (if set to TERM=dumb) as per the same reasons that it was introduced in PR: #906. |
c43f1df
to
3bd11e8
Compare
'"apt update -qq && apt install snapcraft -y && cd $(pwd) && ' | ||
'snapcraft && snapcraft push *.snap --release edge"'), | ||
'docker run -e TERM -v $PWD:$PWD -w $PWD -t snapcore/snapcraft' | ||
' sh -c "snapcraft && snapcraft push *.snap --release edge"'), |
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.
@3v1n0 I think this will also need apt update and apt upgrade, because the snapcore/snapcraft image is not being updated: https://bugs.launchpad.net/snapcraft/+bug/1639112
I like -w instead of cd, and I didn't know about it, thanks. However, being a little pita, there are three bugs here:
I'm ok with fixing the three in the same PR, but my problem is that the description of the PR only mentions 1. The bug mentions 2 and 3. And the PR fixes 1 and 2. :) Also, this is not your fault, but you are changing two things in the code, and only one in the tests. That means that we are missing a test. |
you might want to look at #1125 wrt usage of |
Where does this PR stand? |
Closing due to lack of activity, feel free to reopen once updated. |
And export there the host's TERM variable.
LP: #1659372