Skip to content
This repository was archived by the owner on Jun 29, 2024. It is now read-only.

demo: improve initial startup #33

Closed
wants to merge 1 commit into from

Conversation

DAMEK86
Copy link
Contributor

@DAMEK86 DAMEK86 commented Apr 17, 2024

  • create initial certs, even if no remote SKI is provided
  • auto register remote SKI if provided, otherwise skip it
  • gitignore: ignore created cert files

- create initial certs, even if no remote SKI is provided
- auto register remote SKI if provided, otherwise skip it
- gitignore: ignore created cert files
Copy link
Member

@DerAndereAndi DerAndereAndi left a comment

Choose a reason for hiding this comment

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

I agree this helps a bit, but I do have some notes. Could you please check them?

@@ -30,11 +30,6 @@ func main() {

flag.Parse()

if len(os.Args) == 1 || remoteSki == nil || *remoteSki == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Without providing an SKI the service can not run meaningful right now. One would have to also integrate accepting any (because there is no UX to deny them) incoming remote pairing requests.

Otherwise block should stay.

Copy link
Contributor Author

@DAMEK86 DAMEK86 Apr 18, 2024

Choose a reason for hiding this comment

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

the current implementation isn't able to generate the cert and print the SKI for pairing without knowing the remote ski.
The idea behind that is providing an easier startup for pairing the cemd with a other eebus device.
Therefore the cemd ski is needed if the target is not auto accept the ski.

Copy link
Member

@DerAndereAndi DerAndereAndi Apr 18, 2024

Choose a reason for hiding this comment

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

Then I suggest simply moving this code block before configuration, err := eebusapi.NewConfiguration

demo := democem.NewDemoCem(configuration, *remoteSki)
demo := democem.NewDemoCem(configuration)

if len(os.Args) > 1 && *remoteSki != "" {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't check of the amount of args here, but rather:

if remoteSki != nil && *remoteSki != "" {

If the nil check above stays, this could also be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nil in general isn't needed, because the default remoteSkivalue is an empty string.

@DerAndereAndi
Copy link
Member

DerAndereAndi commented Jun 29, 2024

Let's improve the demos of eebus-go as this repository will now be archived

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants