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

internal/worker/client.go: refactor reading worker ID #4351

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

schuellerf
Copy link
Contributor

Just refactor the usage of the mutex to remove code duplication

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

LGTM, but the commit message isn't very clear, there are two mutexes in the worker client, and it's not calling to the mutex, it's reading the worker ID.

Can you change the commit message to something like:

internal/worker/client: or worker(client): + refactor reading worker ID

Adds a helper function to the worker client instead of redeclaring the same inline function everywhere.

Adds a helper function to the worker client instead of
redeclaring the same inline function.
@schuellerf
Copy link
Contributor Author

LGTM, but the commit message isn't very clear, there are two mutexes in the worker client, and it's not calling to the mutex, it's reading the worker ID.

Can you change the commit message to something like:

internal/worker/client: or worker(client): + refactor reading worker ID

Adds a helper function to the worker client instead of redeclaring the same inline function everywhere.

Thanks - the message was really bad :-/
please check the updated one

@schuellerf schuellerf enabled auto-merge (rebase) September 6, 2024 09:21
@croissanne croissanne changed the title cmd/osbuild-worker/main: refactor call to mutex internal/worker/client.go: refactor reading worker ID Sep 6, 2024
Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Nice work!

@schuellerf schuellerf merged commit bb53f48 into osbuild:main Sep 6, 2024
45 of 49 checks passed
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