-
Notifications
You must be signed in to change notification settings - Fork 123
Elektra Web jenkins deployment #2110
Elektra Web jenkins deployment #2110
Conversation
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 there any way I can test the Jenkinsfile locally?
unfortunately only if you set up a jenkins installation.
Please remove any duplicate code that is introduced with the current commits. there is no need for functions that do exactly the same that just have a different name.
@ingwinlu I re-used the |
scripts/jenkins/Jenkinsfile
Outdated
@@ -902,6 +923,49 @@ def buildHomepage() { | |||
return homepageTasks | |||
} | |||
|
|||
def deployWebUI() { | |||
node("elektrad") { |
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.
did you add this tag? does whatever this tag describe provide the means to execute the code below?
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.
@ingwinlu how do I add tags? or do you mean a git tag?
scripts/jenkins/Jenkinsfile
Outdated
backend.pull() | ||
frontend.pull() | ||
|
||
sh "docker stop -t 5 ${backendName} || /bin/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.
this functionality could be moved into a function as it is now used 4 times in a row
scripts/jenkins/Jenkinsfile
Outdated
node("elektrad") { | ||
docker.withRegistry("https://${REGISTRY}", | ||
'docker-hub-elektra-jenkins') { | ||
def backendName = "elektrad" |
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.
keep in mind those names are deployed without elektra context. if i am a system administrator and i see a webd container i no nothing about it.
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.
but only on our local registry, right? then the elektra context is there, because it is the elektra docker registry.
@@ -902,6 +923,49 @@ def buildHomepage() { | |||
return homepageTasks | |||
} | |||
|
|||
def deployWebUI() { |
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 had hoped we could reuse some of the code in deployHomepage instead of just having a 1:1 copy with some names exchanged.
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 will try to refactor it a bit
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 improve and set "ready to merge" only if the web ui is actually reachable and works.
@@ -110,6 +110,7 @@ Try it out now on: http://webui.libelektra.org:33334/ | |||
- added "create array" button to easily create arrays | |||
- removed confirmation dialog before deletion (undo can be used instead) | |||
- created a docker image: `elektra/web` | |||
- created a task to deploy the demo |
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.
needs (name)
@@ -152,6 +152,10 @@ maybeStage("Deploy Homepage", isMaster()) { | |||
deployHomepage() | |||
} | |||
|
|||
maybeStage("Deploy Web UI", isMaster()) { |
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.
@ingwinlu Is it possible/useful to deploy Homepage/Web UI Demo in parallel?
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.
not really as changes to the docker environment are serialised anyway. plus the maximum speed up that can be gained are around 5s.
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.
@omnidan I mean this "isMaster" here.
scripts/jenkins/Jenkinsfile
Outdated
DOCKER_IMAGES.webui_elektrad = createDockerImageDesc( | ||
"elektra/elektrad-demo", this.&idHomepage, | ||
".", | ||
"./scripts/docker/web/elektrad-demo/Dockerfile", |
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 add some more mountpoints.
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.
did we really add a web directory? super confusing as homepage is not part of web. can you rename to webui?
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.
@markus2330 I added some new mountpoints:
RUN kdb mount /etc/networks system/networks hosts
RUN kdb mount /etc/ssh/ssh_config system/ssh line
RUN kdb mount /etc/ldap/ldap.conf system/ldap line
RUN kdb mount /var/lib/dpkg/available system/dpkg/available dpkg
RUN kdb mount /var/lib/dpkg/status system/dpkg/status dpkg
is this enough or do you have any ideas for other mountpoints?
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.
@ingwinlu I renamed the folder to webui
your approach also is failing to provide your base image. that has to be built as well. your contexts are also wrong. your base image should not git clone but use the existing workspace. |
@ingwinlu what do you mean by "your contexts are wrong"? how can I use the existing workspace in the Dockerfile? |
This line copies the already checked out elektra into the docker build context. Afterwards your other images do not the whole directory as a docker build context. |
6d5837f
to
214dd7b
Compare
@ingwinlu I fixed it (I think). How do I get jenkins to build the webui? |
It gets build automatically. But it seems there are errors because of the release notes: https://build.libelektra.org/jenkins/blue/organizations/jenkins/libelektra/detail/PR-2110/8/pipeline |
@markus2330 @ingwinlu jenkins says:
How can I get it to not skip this step? |
Maybe you cannot trigger it from the PR because of the @ingwinlu What is the best way to do that? |
@markus2330 I will temporarily turn the |
I assume it is for the internal communication: Is it really necessary to have this DNS entry? Nevertheless, elektrad-demo now points to a7. |
7962229 does not what you think it does. it now pulls from hub.docker instead of using the image you build earlier in the pipeline... docker assumes hub.docker if you omit adding a registry to an image tag. |
…/web-base" This reverts commit 7962229.
@ingwinlu thanks for the hint! I reverted the commit. @markus2330 thanks for adding the DNS. yes, it is just for internal communication. Edit: I noticed it still does not seem to resolve the DNS properly, but maybe it will take a while until a7 refreshes its dns cache. https://webdemo.libelektra.org/instances/56755a3c-27f5-45f1-92bd-8140fe1780f0 should work when it's refreshed. |
I also re-enabled full builds and changed the |
RUN kdb mount /etc/ssh/ssh_config system/ssh line | ||
RUN kdb mount /etc/ldap/ldap.conf system/ldap line | ||
RUN kdb mount /var/lib/dpkg/available system/dpkg/available dpkg | ||
RUN kdb mount /var/lib/dpkg/status system/dpkg/status dpkg | ||
|
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.
you really want to join those statements together to speed up builds and reduce image size
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.
this is true for all your images
@@ -859,22 +889,20 @@ def buildPackageDebianStretch() { | |||
}] | |||
} | |||
|
|||
def deployHomepage() { |
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 the strange notation of having to deploy 2 images?
just split it up to 1 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.
this could probably be further improved:
- Instead of passing the imageId I think it is easier to just pass the image description.
- host should probably be renamed to hostname.
scripts/jenkins/Jenkinsfile
Outdated
|
||
def buildWebUI() { | ||
def webuiTasks = [:] | ||
webuiTasks << buildImageStage(DOCKER_IMAGES.webui_base) |
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.
remove here you build it earlier via autobuild
@ingwinlu should be fine now! |
@omnidan hard to say as you already removed the builds from the pr again (and deployment was never tested) |
@ingwinlu it already deployed earlier, but I can let it run again with the adjustments now. |
… full build" This reverts commit b3a392e.
lgtm when the isMaster stuff is back in. thanks for your work on this and for implementing my feedback. |
- added 404 page for invalid instance ids *(Daniel Bugl)* | ||
- implement drag & copy by holding the Ctrl or Alt key *(Daniel Bugl)* | ||
- add button to show error details *(Daniel Bugl)* | ||
- allow deleting all keys in a namespace *(Daniel Bugl)* |
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 it would be enough to add your name once at the very end of the highlight. But this is okay, too.
Do not forget to add the instructions of manual building+uploading of the docker images. (inside some README of the Web UI)
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.
@markus2330 oh okay, the error message told me to add (Name)
after each line. I will add the information to the README of scripts/docker/webui/.
…e-enable full build"" This reverts commit e81ae25.
@ingwinlu @markus2330 done! |
Thank you, great job! |
I copied the definitions from the homepage tasks, so it should work. Is there any way I can test the Jenkinsfile locally?
manually building docker images
make sure to change
1.6
to a new version if it was updated. base image (bold) has to be built first, others can be ran in parallel.scripts/docker/webui/base
run:docker build --no-cache -t elektra/web-base:latest -t elektra/web-base:1.6 .
scripts/docker/webui/elektrad
run:docker build -t elektra/elektrad:latest -t elektra/elektrad:1.6 .
scripts/docker/webui/webd
run:docker build -t elektra/webd:latest -t elektra/webd:1.6 .
scripts/docker/webui/elektrad-demo
run:docker build -t elektra/elektrad-demo:latest -t elektra/elektrad-demo:1.6 .
scripts/docker/webui/webd-demo
run:docker build -t elektra/webd-demo:latest -t elektra/webd-demo:1.6 .
scripts/docker/webui/web
run:docker build -t elektra/web:latest -t elektra/web:1.6 .
to publish these images to docker hub (the main docker registry), simply run:
docker push elektra/web-base
docker push elektra/elektrad
docker push elektra/webd
docker push elektra/elektrad-demo
docker push elektra/webd-demo
docker push elektra/web