Skip to content

upgrades/refactoring#10

Open
silannisik wants to merge 20 commits into
kpn-dsh:masterfrom
silannisik:develop
Open

upgrades/refactoring#10
silannisik wants to merge 20 commits into
kpn-dsh:masterfrom
silannisik:develop

Conversation

@silannisik

Copy link
Copy Markdown

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

Comment thread Dockerfile

ARG IMAGE_VERSION

ARG image_version

Copy link
Copy Markdown

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.

@rutgerc-klarrio rutgerc-klarrio left a comment

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown
Contributor

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