-
Notifications
You must be signed in to change notification settings - Fork 726
[travis] upload packages to transfer.sh and to the Snap Store #1125
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
Conversation
05f7f89
to
3253e36
Compare
Codecov Report
@@ Coverage Diff @@
## master #1125 +/- ##
=======================================
Coverage 71.04% 71.04%
=======================================
Files 201 201
Lines 7374 7374
=======================================
Hits 5239 5239
Misses 2135 2135 Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3253e36
to
3721738
Compare
This comment has been minimized.
This comment has been minimized.
3721738
to
95d9c29
Compare
3ff2bfa
to
95d9c29
Compare
tools/report-build.py
Outdated
import requests | ||
import sys | ||
|
||
HEADERS = {"Authorization": "Bearer {}".format(os.environ["GITHUB_TOKEN"])} |
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.
There's very little exception handling in this script, on purpose - anything out of the ordinary should make it go away.
This comment has been minimized.
This comment has been minimized.
Thanks, this idea sounds really useful. I did not look at the code (so this is not a review) but I have some comments on the topics you raised. |
@townsend2010 care to share your opinion on the topics I raised? |
20cca79
to
42a4d38
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3e2622f
to
4d8c2b5
Compare
This comment has been minimized.
This comment has been minimized.
feb5552
to
092f753
Compare
This comment has been minimized.
This comment has been minimized.
cf03038
to
f059020
Compare
This comment has been minimized.
This comment has been minimized.
04a1dc8
to
6dc6d82
Compare
This comment has been minimized.
This comment has been minimized.
- do not `snapcraft login` if not publishing - do not run covreport on failure - rename `build_id` - streamline `name`/`owner` determination - add more timeline event types Co-authored-by: Ricardo Abreu <[email protected]>
- rename report_build to snake-case - reorder imports - fix long line
On merges from a fork, credentials are unavailable.
dc179d4
to
96cfca8
Compare
KeyError is too big of a net.
transfer.sh, when in a bad state, causes curl to hang indefinitely.
96cfca8
to
267deb9
Compare
UX detail (I should say DX here, for developer 😛): I think the messages look a bit more tangled up in appearance after the snap command. How would you feel about changing the format to something like:
|
Yes, in a further PR. Note I'm matching the build names with a simple |
This comment has been minimized.
This comment has been minimized.
query GetEvents($owner: String!, | ||
$name: String!, | ||
$pull_request: Int!, | ||
$count: Int!) { |
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 wish our clang-format accepted this sort of formatting in C++, it often makes for much more readable code.
Snap build available: |
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.
Unfortunately I seem to have saturated transfer.sh again… It shouldn't be that bad when I stop experimenting with this, but will look around for an alternative for the future.
- '[[ "${TRAVIS_EVENT_TYPE}" != "pull_request" || "${TRAVIS_SECURE_ENV_VARS}" == "false" ]] && exit' | ||
- BUILDS=() | ||
- >- | ||
[ -n "${MULTIPASS_SNAP_CHANNEL}" ] | ||
&& snapcraft login --with ${HOME}/snap-login | ||
&& snapcraft push multipass_*.snap --release ${MULTIPASS_SNAP_CHANNEL} | ||
&& BUILDS+=("\`snap refresh multipass --channel ${MULTIPASS_SNAP_CHANNEL}\`") | ||
- CURL_OUTPUT=$( timeout 5m curl --fail --upload-file *.snap https://transfer.sh/ ) | ||
&& BUILDS+=("[$( basename ${CURL_OUTPUT} )](${CURL_OUTPUT})"); | ||
echo ${CURL_OUTPUT} | ||
- ./tools/report_build.py Snap "${BUILDS[@]}" |
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 now wonder whether we should do this on the Debug build? The snap checks and now the uploads mean that this job runs almost twice as long as the Debug one.
And maybe it'd be better for debugging to have Debug builds out of PRs?
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 don't know, I don't have a strong opinion on that. Release would be closer to the actual thing, but debug would indeed be more practical. Would that affect actual releasing? And aren't the snap checks useful in the build that uploads the snap? What is the gain in having debug and release builds switching places in what concerns duration?
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.
Would that affect actual releasing?
No, for releases (RC we'd have to decide on) we'd build RelWithDebInfo still.
And aren't the snap checks useful in the build that uploads the snap?
At worst they would fail to upload. Now I think of it maybe we can drop the review step as the store does it for us… The snap upload would have to move up to script:
though, so failure to release would be a CI failure, and alternate with review-tools
if not publishing.
What is the gain in having debug and release builds switching places in what concerns duration?
One would do the snap review check, another would do the upload.
Any case, something to ponder for the future.
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.
Alright
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.
At least the "faster" concern is moot now: #1141
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.
bors r+
1125: [travis] upload packages to transfer.sh and to the Snap Store r=ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>
Build succeeded |
1125: [travis] upload packages to transfer.sh and to the Snap Store r=ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>
1125: [travis] upload packages to transfer.sh and to the Snap Store r=ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>
1125: [travis] upload packages to transfer.sh and to the Snap Store r=ricab a=Saviq Co-authored-by: Michał Sawicz <[email protected]>
No description provided.