Skip to content

Add chart for HawkBit#18

Merged
ctron merged 1 commit intoeclipse-packages:masterfrom
kiwigrid:hawkbit
Jan 23, 2020
Merged

Add chart for HawkBit#18
ctron merged 1 commit intoeclipse-packages:masterfrom
kiwigrid:hawkbit

Conversation

@axdotl
Copy link

@axdotl axdotl commented Jan 9, 2020

Signed-off-by: Axel Köhler axel.koehler@kiwigrid.com

This PR will add the Hawkbit update server chart to the packages project.
Feedback is highly appreciated.

@axdotl axdotl requested review from calohmn and ctron as code owners January 9, 2020 13:20
@thjaeckle thjaeckle requested a review from laverman January 9, 2020 13:25
@thjaeckle
Copy link
Contributor

Review required by @laverman as hawkBit representative in the Packages project

@monotek
Copy link
Contributor

monotek commented Jan 9, 2020

The problems seems to be usage of rabbitmq chart from bitnami repo, which is not added in ct.yaml
Adding bitnami repo like in the link below should do the trick:
https://github.com/kiwigrid/helm-charts/blob/master/.ci/ct.yaml#L5

Copy link
Contributor

@laverman laverman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this contribution here! 👍

Just some general comments:

  • Would you mind adding a license header to the *.yaml files?
  • I triggered the CI-actions again, to see if #17 fixed the failed check.
  • I don't want to start a big naming discussion, whether to name it "eclipse-hawkbit" or "hawkbit-update-server" ;-) However, in order to be consistent with ditto, it might make sense to go with the former (knowing that hawkBit named its docker image "hawkbit-update-server") ... What do you think?
  • Also in accordance with ditto, shall we add a Local Setup section to the readme as well? (Could also be part of another PR)

Please find my other comments in the respective files.

env:
- name: SPRING_PROFILES_ACTIVE
value: "{{ .Values.spring.profiles }}"
- name: MANAGEMENT_SERVER_PORT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this property used for?
If you want to change hawkBit's server port SERVER_PORT would be the correct property.

Copy link
Author

@axdotl axdotl Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It configures the SpringBoot management server port. The port is used for the probes.
(ref: https://docs.spring.io/spring-boot/docs/current/reference/html/production-ready-features.html#production-ready-customizing-management-server-port)

If you agree I'll keep this confiugration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. However, as there are no actual actuator endpoints (e.g. health, metrics, ...) available, I was wondering whether to just use the hawkBit landing page for the probes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it.

- name: http
containerPort: 8080
protocol: TCP
- name: management
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above.

## Configuration for the device management federation
## ref: https://www.eclipse.org/hawkbit/apis/dmf_api/
dmf:
hono:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following configuration requires eclipse-hawkbit/hawkbit#890 to be merged. I'm not sure, if we should include it right away, since 0.3.0-M5 doesn't support it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm way unsure, but might you mean this PR eclipse-hawkbit/hawkbit#865?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The properties in this section are not supported by hawkBit as of now. They will be introduced with eclipse-hawkbit/hawkbit#890 as they configure Hono to be used as device registry for hawkBit via DMF. As these properties will have no effect at the moment, I was wondering whether to exclude them for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented the properties and added a reference to the related PR.
Is this okay for you?

@monotek
Copy link
Contributor

monotek commented Jan 15, 2020

Please don't merge. I want to try to fix the ci. Currently pvc were disabled.

@monotek
Copy link
Contributor

monotek commented Jan 15, 2020

CI works now with persistence by using a forked kind action https://github.com/eclipse/packages/pull/18/files#diff-60fe268f71ae7f8409aff40d0a3e51d8R61

I've create a pr at the helm folks kind action repo to get it merged: helm/kind-action#16

Not sure if we want to go with my temporary version or wait for their merge.

@ctron
Copy link
Contributor

ctron commented Jan 16, 2020

Let's wait a day or two and see how it goes.

@ctron
Copy link
Contributor

ctron commented Jan 16, 2020

On another topic: When using the original helm charts for hawkbit from kiwigrid, I had to make some changes to be able to run with non-root containers. Is root still a requirement, or can we maybe get rid of that requirement from the start? I made a few modifications in order to drop the requirement, but I don't want to mess up your efforts 😉

@monotek
Copy link
Contributor

monotek commented Jan 16, 2020

+1 for non root

As we're waiting anyway, we could maybe add the changes here?

Do you have a link to your changes?

@ctron
Copy link
Contributor

ctron commented Jan 16, 2020

Not to the changes, but to the content: https://github.com/ctron/hawkbit-kubernetes

@sophokles73
Copy link
Contributor

@axdotl @monotek This PR now seems to contain arbitrary changes to all kinds of artifacts, e.g. the general IoT Packages README, the CI pipeline and, last but not least, the hawkBit chart itself which originally seemed to be the subject of this PR :-)
I would appreciate if we could create separate PRs for the former two things as they do not seem to be related to the hawkBit chart per se, or are they?

@monotek
Copy link
Contributor

monotek commented Jan 17, 2020

Sure, no problem. I will create separate pr for the ci stuff.

@monotek
Copy link
Contributor

monotek commented Jan 17, 2020

i've created the pr: #23

@axdotl
Copy link
Author

axdotl commented Jan 17, 2020

@sophokles73 With respect to the charts/README.md: @laverman asked for some info about a local setup. As this is similar to the setup described in the README of the Ditto chart, I assumed that this might be true for all charts in the package project. That's why I decided to provide some general info about a local setup.
What do you think: Should I move it to charts/hawkbit/README.md or should it remain in current location?

Next to that I followed @sophokles73 advice and reverted the CI related changes.

What do you think - can we proceed and get this PR merged once @laverman re-reviewed it?

Copy link
Contributor

@monotek monotek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think persistence should be enabled by default.
Using empty dir is potentially dangerous and should only be used if you activly enable it.

@ctron
Copy link
Contributor

ctron commented Jan 22, 2020

Could you re-base the PR?

@ctron
Copy link
Contributor

ctron commented Jan 23, 2020

I added a small PR for this PR -> kiwigrid#1 … This drops the default requirement to run as user 1001, and lets the cluster assign a random user uid, which makes it work on OpenShift. You can of course always re-enable the uid 1001 requirement.

@monotek
Copy link
Contributor

monotek commented Jan 23, 2020

I've added the needed stuff to run the container as non root user now.

But as already mentioned in the comment to your pr (kiwigrid#1 (review)) i don't like to disable this by default.

Signed-off-by: André Bauer <andre.bauer@kiwigrid.com>
Copy link
Contributor

@ctron ctron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the PR needs a rebase, but 👍 in any case.

@monotek
Copy link
Contributor

monotek commented Jan 23, 2020

I still think the PR needs a rebase, but in any case.

done.

@ctron ctron merged commit 71e79fc into eclipse-packages:master Jan 23, 2020
@ctron
Copy link
Contributor

ctron commented Jan 23, 2020

Yay! Thanks for all the work!

@axdotl axdotl deleted the hawkbit branch January 23, 2020 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants