Skip to content
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

upgrades/refactoring #10

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

upgrades/refactoring #10

wants to merge 20 commits into from

Conversation

silannisik
Copy link

Hi,

We've been using this repository for onboarding new teams on DSH.
The changes we made to the repo are included in this PR.

  • Updated Docker plugin to fabric8io and specifying the platform as linux/amd64, otherwise the container doesn't work when building on M1 chips and running it in a tenant
  • Added an example folder with a script to subscribe and publish using MQTT with reference to the DSH documentation and example output
  • Upgraded Java version (8->17)
  • Refactored code, put classes into separate packages, removed unused imports, fixed typos
  • Pass uid:gid as build arguments in Dockerfile, so we don't need to change the source when using it in different tenants

MAINTAINER Bruno De Bus <[email protected]>

ARG IMAGE_VERSION

ARG image_version

Choose a reason for hiding this comment

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

I would keep the uppercase ARG names. I believe in docker files it is the more popular style.

Copy link

@rutgerc-klarrio rutgerc-klarrio left a comment

Choose a reason for hiding this comment

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

I understand that it is example code, but it might still be worth it to look at using a logging library iso using print statements.

Copy link
Contributor

@gert-verbruggen-klarrio gert-verbruggen-klarrio left a comment

Choose a reason for hiding this comment

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

See the remarks of Rutger about the uppercase IMAGE_VERSION and the logger.

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.

3 participants