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

Fix app discovery endpoint #4421

Merged
merged 2 commits into from
Nov 7, 2024

Conversation

uncleDecart
Copy link
Member

This PR removes ioutil from msrv_test and fixes bug in discovery route: flag allowToDiscover which is set in EVE-API was disregarded before, i.e. all the application instances were allowed to discover other app instances, now discovery service comply to that flag

@uncleDecart uncleDecart requested review from milan-zededa and removed request for eriknordmark November 6, 2024 10:21
@uncleDecart uncleDecart added bug Something isn't working go Pull requests that update Go code labels Nov 6, 2024
Copy link
Contributor

@milan-zededa milan-zededa left a comment

Choose a reason for hiding this comment

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

Sorry about that :)
I think this should be also backported to 13.4 (so the PR should have the stable tag)

@uncleDecart uncleDecart added the stable Should be backported to stable release(s) label Nov 6, 2024
@rene
Copy link
Contributor

rene commented Nov 6, 2024

Triggering tests.... @uncleDecart , I understand the change for the first commit is pretty simple, but appreciate if you could think in a phrase to get rid of the lint error....

@uncleDecart
Copy link
Member Author

@rene I thought that commit message linter was more of a suggestion than the rule and its' purpose to make make developer think about commit message so that other would understand what has been done on a high level.
What would you write in the body of that commit? I can write the same message, but I don't see how it would be beneficial for any of us 🤷‍♂️

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

@eriknordmark
Copy link
Contributor

@uncleDecart please fix the commitlint issue
Commit 60cb744 has no body.

removing deprecated ioutil from tests and adding commit body to pass linter

Signed-off-by: Pavel Abramov <[email protected]>
Previously, flag AllowToDiscover was disregarded, which is not
the indented behaviour

Signed-off-by: Pavel Abramov <[email protected]>
@rene
Copy link
Contributor

rene commented Nov 7, 2024

@rene I thought that commit message linter was more of a suggestion than the rule and its' purpose to make make developer think about commit message so that other would understand what has been done on a high level. What would you write in the body of that commit? I can write the same message, but I don't see how it would be beneficial for any of us 🤷‍♂️

Well, commit 0aa72b8 is a good example for the same kind of change:

io/ioutil is deprecated
As of Go 1.16, the same functionality is now provided by package io
or package os, and those implementations should be preferred in new
code.

Since we are bombarded with Yetus complains about ioutil in all our
PRs, it is worth getting rid of it.

Signed-off-by: Milan Lenco <[email protected]>

You can just use this phrase as the body of the commit:

As of Go 1.16, the same functionality is now provided by package io
or package os, and those implementations should be preferred in new
code.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Run tests

@eriknordmark eriknordmark merged commit bd0f057 into lf-edge:master Nov 7, 2024
29 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code stable Should be backported to stable release(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants