-
Notifications
You must be signed in to change notification settings - Fork 111
Conversation
Can one of the admins verify this patch? |
0d767c9
to
c71f523
Compare
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.
Hello, I think we have too many duplicates
Also there is little value of using yet another CI than github actions.
Travis job should do the bare minimum: only build for arch that does not work with qemu (so no need to have linelint checks, etc)
Scripts are doing
docker pull quay.io/eclipse/che-theia-dev:next
this image is not multi-arch so I don't see the benefits
I don't see any reason of building amd64 image using travis. Using github action is working just fine.
happy path scripts should not be part of che-theia. in Che it's using separate github actions repositories
FYI @nickboldt . |
@benoitf it would be more productive to ask questions than to overtly state "there is little value of using yet another CI than github actions". The value is that GH actions w/ qemu+s390x do not work, and there's no plan in place in any known timeline to add support for that architecture, or to add the option for hosted runners on more arches. Meanwhile Travis already has s390x hardware, provided by IBM. So if building Che on Z is something that Project Management and Product Management care about, then some solution must be found, even if it means a hybrid solution (multiple CI tools, perhaps doing builds on one and tests on another) or a migration solution (move everything to Travis to maintain a single pipeline for all supported arches). |
I already commented that it should begin ONLY with arches that may not work with githubs action, not duplicating all github actions. (For example there is no value by this PR on checking linelint from source code with travis CI as it's already checked by github actions) |
As TravisCI support triggered job so we could just call within github action the call to travis-ci to build the expected build on a given arch |
Why? If Travis works I'd propose we move all building for all arches there, and leave GitHub action for small checks like linters. What was discussed during the community call was that we could try building all arches on Travis (in parallel to GitHub actions) for a few weeks. During that time we can evaluate whether it's working smoothly. If there are no major issues we could disable GitHub action builders after that. |
There is no design in Travis CI to share easily component across repositories like the actions in github. I've seen that limits could be increased but most of the time the limits are 2 or 3 jobs at once per organization (vs 20 jobs in Github Actions) Pre-installed tools are lagging behind github actions runner so it means more time to install stuff on each job (and then delay result) (for example minikube, etc.) Also I think you need to be a committer to respin jobs vs just being the author of the PR on github action |
If this is all about happy path tests, can't we leave that on GitHub action and just use Travis for the build + publish? |
It's about PR checks as well. If almost all jobs are queued, then you need to wait a very long time to be able to merge a PR. |
I'm fine with using GitHub actions to trigger travis, then. However I'd like to avoid building and merging of arches separately -- can we build all arches on travis (triggered by GitHub actions)? |
Anyway without buildx it'll build separate images and then merge them together at the end (creating so I don't see the issue if it's on the same host or not ? |
If we do all builds in travis (vs. using GH to trigger Travis for the 1 or 2 arches we can't build w/ buildx), then we still need to know when the triggered travis processes are complete and the image:tag-s390x image exists, so it can be merged into the overall manifest with So... how does GH know that the travis-delegated process has completed? Does it listen/wait for a return code? or do we have to loop & wait for the new image to exist, and then trigger the manifest creation with amend? If delegation works then I could see a hybrid solution of (gh actions + buildx for some arches) + (delegation to travis for other arches) working for us... the only concern I would then have is that we would have a team of devs/qe/doc who have to know how to run builds using multiple build systems. Using travis for everything would simplify the layers of tech we have to know/worry about... but I suppose since moving everything to travis isn't going to happen (eg., for linters) then we'll already be in a hybrid mode regardless of whether we use TL;DR, if we can delegate to Travis and easily get status when the s390x build completes, I'm okay with this hybrid build-and-assemble approach. |
Codecov Report
@@ Coverage Diff @@
## main #1127 +/- ##
==========================================
- Coverage 32.78% 32.69% -0.10%
==========================================
Files 290 295 +5
Lines 9885 10005 +120
Branches 1457 1550 +93
==========================================
+ Hits 3241 3271 +30
+ Misses 6641 6625 -16
- Partials 3 109 +106
Continue to review full report at Codecov.
|
@nickboldt https://docs.travis-ci.com/api#builds |
So it's a poll-only approach? I was hoping for a push-notification approach, such that the travis process can say "I'm done!" and the GH process that invoked it can then continue its flow.
rather than
But I guess we already do that with Che overall release process, polling quay for the existance of new che image tags before moving to the next step in the pipeline (eg., don't start Che operator until DWCO image exists). We could either poll travis or just skopeo inspect quay for the tag(s) we expect, and wait until some timeout before failing the build. |
you can trigger events from travis to github as well (as we do for che-release process) |
I think it depends on how you want to see the result in PR checks / dependencies. |
Ah, so the flow would be:
That's a nice way to split things up, as long as the secrets are in place at both ends for cross-infra triggering. |
Notes from today's call w/ Florent and other Che devs:
@cwsolee can you talk to your people at Travis to ask if we can get more capacity than 3 parallel jobs? If that's a hard limit, then we can't use travis for more than Z builds and it makes sense to keep everything else in the 20-build GHA pool (x, arm, power, etc.), and have a GH action delegate to Travis for only the s390x arch builds. |
Travis change our concurrency limit too, it's 20 now and Travis were quite willing to increase more when needed. With both time limit & concurrency limit increase, we haven't hit any problem since then. |
c71f523
to
8c46ce0
Compare
Changes implemented:
Made below changes to the workflows in Travis, as they were specific to amd64 and were not related to image build & push:
PR check limitation on triggering PR-check jobs on Travis via API:
While checking on the implementation, we found that the Travis request API doesn't support triggering builds on PR references.
|
@nickboldt & @ericwill , We keep this PR as draft for u to take a look before we submit, let us know whether you've any comments. Comments from others are welcome too. |
.travis.yml
Outdated
|
||
env: | ||
global: | ||
- TAG=next |
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 think the tag would conflict there with current one as we want to experiment for a while how it goes ?
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.
is that why we have the TRAVIS_TAG to append on for now, and later set to "" ?
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 think the tag would conflict there with current one as we want to experiment for a while how it goes ?
TAG won't conflict with the current next
tag as while publishing the image we have suffixed the next tag with TRAVIS_TAG (i.e. next-travis
).
is that why we have the TRAVIS_TAG to append on for now, and later set to "" ?
Yes. TRAVIS_TAG is introduced as Travis is experimental. Later this flag can be removed completely from all files.
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 still don't get why we do need an addition TRAVIS_TAG variable
if TAG is set to next-travis
then image name will be suffixed by this tag so it should be enough to change the suffix
.travis/publish_multiarch.sh
Outdated
@@ -0,0 +1,36 @@ | |||
#!/bin/bash |
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.
copyright header is missing but it also don't pass shellcheck.net
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.
can we move this folder to .ci/ folder (as we have a generic folder)
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.
Yes. Will surely move it to .ci/ folder
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.
Will add the Copyright header and make necessary changes. We can include all variables in double quotes to avoid shell check error except $AMEND.If we add $AMEND in quotes we get the below error
$ docker manifest create "${REGISTRY}/${image}:latest-${TRAVIS_TAG}" "${AMEND}"
invalid reference format
To avoid shellcheck in case of $AMEND, we can include # shellcheck disable=SC2086
in publish_multiarch.sh
Let us know your thoughts on the same
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.
could you just use eval at the beginning of the line ?
$ eval docker manifest create "${REGISTRY}/${image}:latest-${TRAVIS_TAG}" "${AMEND}"
Since we have a replacement PR now in play, I'm going to close this in favour of #1197 |
reopening -- my changes in #1197 were not quite ideal, so we can keep using this one (or open a new one, whatever's better) |
0a318f3
to
4298cb6
Compare
4298cb6
to
84c979a
Compare
84c979a
to
aee3cfa
Compare
@nickboldt I have updated the PR with changes from #1197. |
apologies, but ran out of time this week to review. I'm on PTO next week; perhaps @azatsarynnyy or @benoitf want to merge this? travis secret and branch rules are set up the same way for theia as for machine-exec, so once merged this should fire the same way as the machine-exec one. |
Signed-off-by: vibhutisawant <Vibhuti.Sawant@ibm.com>
-Added SHA1_SUFFIX to build.include to differentiate between GA and Travis builds. -Added copyright header -Moved publish_multiarch.sh to .ci directory -Removed $TRAVIS-TAG variable from build files
aee3cfa
to
b336e87
Compare
Hi @nickboldt |
I've uploaded my travis API token from https://app.travis-ci.com/account/preferences to the che-theia repo... and we're in better shape. But see news in #1216 -- we have to merge that change before we can get any passing docker builds. |
Good news: ubi and node builds now passing on 390x and ppc64le. https://app.travis-ci.com/github/eclipse-che/che-theia/jobs/537152748 Looks like timeout/stalled process. Do we actually need alpine builds for s390x and ppc64le? Why not just do ubi8 ones? @benoitf WDYT? |
alpine images are multi-arch as well so I see no reason to not have them as well |
just seems redundant since we only care about ubi8 on power and z as a way of seeing problems coming to CRW sooner. |
-- https://app.travis-ci.com/github/eclipse-che/che-theia/jobs/537152749 Alpine build in travis on s390x failed 3 times now. Can we skip it? disable it? fix it? throw rocks at it? :D |
Having increased the worker node's memory, travis was successful: https://app.travis-ci.com/github/eclipse-che/che-theia/builds/237558344 |
Hello, |
@cwsolee said:
Given repos under eclipse-che are managed by Eclipse (eg., eclipse-che/che#19391 -> https://bugs.eclipse.org/bugs/show_bug.cgi?id=571958 ), and that Eclipse Foundation will stop supporting Travis CI on GitHub organizations it manages... [and] won't configure any new repository / organization and [they] plan on removing the TravisCI GitHub app from all organizations [they] manage on October 20th.... I'd say that likely means we won't be able to get more eclipse-che/* projects into Travis under the Eclipse Foundation mandate. But... hey, maybe you can reach out to the Eclipse Webmaster? or @cwsolee can talk to the Travis folks to find out if "add credit if needed" will actually be easy? |
Signed-off-by: vibhutisawant Vibhuti.Sawant@ibm.com
What does this PR do?
Adds Travis CI support for Che-theia.
What issues does this PR fix or reference?
This refers to issue-19688. Adds Travis CI support to build multi-arch images.
Workflows migrated from GA to Travis CI:
PR check and Happy path tests
Check a Theia branch
Publish-built-in-extension-report
Build and publish next
Release
Note: TAG is a required field in the above config.
Check a Theia branch and Release workflow will require a manual build by using the trigger build option from Travis UI. After selecting a branch, we can provide a custom config.
Env variables to be set in Travis CI settings:
DOCKER_PASSWORD
DOCKER_USERNAME
QUAY_PASSWORD
QUAY_USERNAME
CHE_BOT_GITHUB_TOKEN
GITHUB_TOKEN
CHE_NPM_AUTH_TOKEN
MATTERMOST_WEBHOOK_URL
GITHUB_ACTOR