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

net_kernel:allowed/0, specs and docs #8207

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MarkoMin
Copy link
Contributor

Few changes:

  1. net_kernel:allowed/0 is speced and documented. Notice that it is already listed in "documented API exports", but for some reason docs were missing. I think it's a must to have it documented, because if you don't then the user can't figure out if net_kernel:allow/1 was called in the past, therefore blocking nodes with the same cookie to connect (which I believe can be very frustrating and and to debug!). This is just a pure getter function, I see no threat exposing it.
  2. net_kernel:allow/1 was missing ignored value it can return. You'll get it if e.g. your node is started without (s)name.
  3. 2 small typos in comments

Copy link
Contributor

github-actions bot commented Feb 29, 2024

CT Test Results

    2 files     67 suites   1h 2m 24s ⏱️
1 528 tests 1 284 ✅ 244 💤 0 ❌
1 729 runs  1 437 ✅ 292 💤 0 ❌

Results for commit 2ec2466.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Mar 4, 2024
@kikofernandez
Copy link
Contributor

As per the new documentation, this function has the specs but was not displayed as a public function, which is why it had the -doc false.. By removing the doc false. you are making this a public function.

We will get back to you once we have determined if this function should be public or not.
If it becomes public, then it needs to be also tested, which I think is not the case at the moment.

@MarkoMin
Copy link
Contributor Author

MarkoMin commented Mar 7, 2024

By removing the doc false. you are making this a public function.

That was my intention because I think that user must have an option to retrieve the state of allowed node.

If it becomes public, then it needs to be also tested, which I think is not the case at the moment.

I definitely agree, but FWIW net_kernel:allow/1 is currently public, but not tested (searching net_kernel:a in test dir gives no results). We definitely need to test both functions. IMHO either both functions should be public or none.

@kikofernandez
Copy link
Contributor

I think both functions make sense as public.
For the PR to be accepted, we need some test cases.
Thanks!

@MarkoMin
Copy link
Contributor Author

I've added the testcase that goes through 4 different cases:

  1. Different cookies, not allowed - no connection
  2. Same cookie, not allowed - no connection
  3. Same cookie, allowed - connection
  4. Different cookie, allowed - connection

@kikofernandez kikofernandez self-requested a review April 6, 2024 11:58
Copy link
Contributor

@kikofernandez kikofernandez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you add tests to the allow function as well?
I see that you use it via rpc:call but that, unfortunately, we do not have tests for it (AFAIK). Apart from this, the type of allow seems to be wrong, since it can return ignored.

Thanks again for the PR

@MarkoMin
Copy link
Contributor Author

MarkoMin commented Apr 7, 2024

Could you add tests to the allow function as well?

Both allow/1 and allowed/0 functions are used in tests. They are literally connected so I don't see a reason to have separate tests for both, I focused on both of them being covered.

I see that you use it via rpc:call but that, unfortunately, we do not have tests for it (AFAIK).

I used it because it is already used in other tests in this suite.

Apart from this, the type of allow seems to be wrong, since it can return ignored.

ignored is added here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants