Skip to content

Fix publisher executor shutdown state#881

Merged
MertCingoz merged 4 commits intofaucetsdn:masterfrom
MertCingoz:mqtt-executor
Jul 23, 2024
Merged

Fix publisher executor shutdown state#881
MertCingoz merged 4 commits intofaucetsdn:masterfrom
MertCingoz:mqtt-executor

Conversation

@MertCingoz
Copy link
Collaborator

No description provided.

@MertCingoz MertCingoz requested a review from grafnu May 7, 2024 10:18
Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

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

Overall technically this looks good, but what kind of testing have you been able to do on this? I took a quick look at your GitHub and it doesn't seem to be setup to run anything, and the upstream faucetsdn project doesn't run everything on a PR (security reasons, mainly)...

@MertCingoz MertCingoz marked this pull request as ready for review July 22, 2024 10:29
@MertCingoz
Copy link
Collaborator Author

@grafnu I have this issue again on local docker setup.

Steps to reproduce:
Local: After containers running and client connected, run registrar one more time.
Cloud: Connect client to MQTT broker then turn off wifi or ethernet adapter.

I hope we can now run local test on this repo. If not is there any guide how to setup CI to only run local test?

@grafnu
Copy link
Collaborator

grafnu commented Jul 22, 2024 via email

@MertCingoz
Copy link
Collaborator Author

I guess my Github username is not docker registry friendly.
https://github.com/MertCingoz/udmi/actions/runs/10042673525/job/27753661849

@MertCingoz MertCingoz requested a review from grafnu July 22, 2024 16:10
Copy link
Collaborator

@grafnu grafnu left a comment

Choose a reason for hiding this comment

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

LGTM. Reminder that for merging with master please remove all commit comments (just the PR title) when you squash-commit... the actual commit log (which is the default GitHub fills in) is pretty useless and just pollutes the commit log!

@MertCingoz MertCingoz merged commit 9347887 into faucetsdn:master Jul 23, 2024
@MertCingoz MertCingoz deleted the mqtt-executor branch July 23, 2024 08:34
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.

2 participants