-
Notifications
You must be signed in to change notification settings - Fork 62
Cico planner #2473
Cico planner #2473
Conversation
it verifies the code runs unit tests runs func tests build the fabric8-ui using new planner changes push to devshift.registry
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-3 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-4 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
cico_run_tests.sh
Outdated
docker build -t fabric8-planner-builder -f Dockerfile . | ||
# User root is required to run webdriver-manager update. | ||
# This shouldn't be a problem for CI containers | ||
docker run --detach=true --name=fabric8-planner -v $(pwd)/fabric8-ui-dist:/home/fabric8/fabric8-planner/fabric8-ui-dist:Z --cap-add=SYS_ADMIN -t fabric8-planner-builder |
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.
docker run
returns the ID of the container being run. It would be nice if you don't specify the name fabric8-planner
but instead use the ID below in docker exec fabric8-planner
.
cico_run_tests.sh
Outdated
# Following steps will create a snapshot for testing | ||
|
||
# Build and integrate planner with fabric8-ui | ||
docker exec fabric8-planner npm pack dist/ |
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.
Instead of running each command individually you might as well mount a file to the fabric8-planner
image and execute that. It probably is cleaner.
fabric8-ui-dist/package.json
Outdated
@@ -0,0 +1,215 @@ | |||
{ | |||
"name": "fabric8-planner", |
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.
Why is this file added here? It looks pretty much like something that exists in another place as well and maintaining multiple versions in different locations is too hard.
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.
accident!
fabric8-ui-dist/package.json
Outdated
"npm": ">= 5.3.0" | ||
}, | ||
"dependencies": { | ||
"@angular/animations": "^4.4.0", |
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.
Don't you want to use sticky versions? @michaelkleinhenz what do you think?
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-5 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-6 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-7 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-8 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-10 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-11 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 Your image is available in the registry: |
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.
We need to cleanup the docker file and the cico_run_tests.sh file. Let's do that in a separate PR.
Let's merge this 🚀
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-53 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-54 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 Your image is available in the registry: Run |
cico_run_tests.sh
Outdated
# Chrome crashes on low size of /dev/shm. We need the --shm-size=256m flag. | ||
CID=$(docker run --detach=true \ | ||
--shm-size=256m \ | ||
-v $(pwd)/fabric8-ui-dist:/home/fabric8/fabric8-planner/fabric8-ui-dist:Z \ |
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.
Please change to: -v $(pwd)/fabric8-ui-dist:/home/fabric8/fabric8-planner/dist:Z \
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.
hi @kwk while we were discussing I forgot, this dist
we want on the host is not from planner. It is from fabric8-ui directory that we have cloned inside the container. So, I do not see other option than to copy it ATM
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.
@pranavgore09 you could simply mount two directories into the container :)
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 we talking about something like this (or am I missing something): https://github.com/fabric8-ui/fabric8-planner/blob/master/scripts/bootstrap.sh#L102
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.
@debloper I'm just saying that we can have one dist for planner and one dist for platform mounted into the container upon start.
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.
@kwk I'm not sure why do you want to mount 2 directories. We do not need to mount two directories. Right now we mount a temporary directory
with name fabric8-ui-dist
into the container. Once planner is integrated into fabric8-ui
; we build the new fabric8-ui
(using npm run build:prod) and copy the final dist
folder from fabric8-ui to the host.
We use this dist
directory in the host to build the docker image.
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.
@debloper Nope. We mount the volume for a completely different purpose.
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.
Please clarify the purpose, so that we're on the same page. Something seems off here.
cd fabric8-ui && npm run build:prod | ||
''' | ||
# Copy dist and Dockerfile.deploy to host (via mounted dir) | ||
docker exec $CID bash -c 'cd fabric8-ui; cp -r dist/ /home/fabric8/fabric8-planner/fabric8-ui-dist/' |
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.
No need to copy because it is already mounted to the correct place (see https://github.com/fabric8-ui/fabric8-planner/pull/2473/files#r176723542).
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.
pease see #2473 (comment)
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.
@pranavgore09 LGTM
# User root is required to run webdriver-manager update. | ||
# This shouldn't be a problem for CI containers | ||
# Chrome crashes on low size of /dev/shm. We need the --shm-size=256m flag. | ||
CID=$(docker run --detach=true \ |
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.
Maybe add -u $(shell id -u $(USER)):$(shell id -g $(USER))
?
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-55 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 Your image is available in the registry: Run |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-56 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 Your image is available in the registry: Run |
@pranavgore09 Your image is available in the registry: Run |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-57 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
[test] |
@pranavgore09 The |
@pranavgore09 Your image is available in the registry: Run |
1 similar comment
@pranavgore09 Your image is available in the registry: Run |
@pranavgore09 Your image is available in the registry: Run |
@pranavgore09 fabric8/fabric8-ui:SNAPSHOT-PR-2473-60 is deployed and available for testing at https://fabric8-ui-fabric8-ui-pr-2473-fabric8-planner.badger.fabric8.io |
@pranavgore09 Your image is available in the registry: Run |
Run build + unit + func tests on cico