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

pytests: extend the offline mode testcase #7675

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

m-schmoock
Copy link
Collaborator

@m-schmoock m-schmoock commented Sep 17, 2024

Offline mode working properly?

Description

I discovered that a node even and specifically when started --offline mode still tries to acquire a listening socket (and would fail to start if it is already bound!)
Also noteworthy, that an --offline node can make outbound connections when done via CLI (or by a plugin like clboss or similar).

What this PR does

Extend the testcase a bit and make it an xfail for the time being.

What needs to be done

I think that an offline node needs to be really offline, because it may harm developers when trying to debug real world data 'in offline mode' and then a connection is made by mistake and a live network channel can end up in an invalid state.

This means that it should not try to bind a listening socket as well as forbid to make manual connections via cli, because a plugin that does not know the node is 'offline' can accidentally make a connection.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK f0c529e

@vincenzopalazzo vincenzopalazzo enabled auto-merge (rebase) September 17, 2024 16:23
@vincenzopalazzo vincenzopalazzo merged commit 02176f7 into ElementsProject:master Sep 17, 2024
36 of 38 checks passed
@m-schmoock
Copy link
Collaborator Author

That was quick, I thought we would add code that fixes the test and extends a bit (disallowing manual connection attempts)

@vincenzopalazzo
Copy link
Collaborator

That was quick, I thought we would add code that fixes the test and extends a bit (disallowing manual connection attempts)

Mh the PR was not ready? How do you want to extend it?

@m-schmoock
Copy link
Collaborator Author

Mh the PR was not ready? How do you want to extend it?

Like this: #7676
And also fix the actual code, so we don't merge in an xfail.

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