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.

Sign up for GitHub

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

Jump to bottom

Ks/container indications #1305

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Ks/container indications #1305

merged 1 commit into from
Sep 11, 2023

Conversation

KSchopmeyer
Copy link
Contributor

@KSchopmeyer KSchopmeyer commented May 25, 2023 •
edited
Loading

Waiting on PR #1313

See commit message for details:

Fixes issue with pywbemlistener being bound to only "localhost" and adds an option to pywbemlistener start and run to allow the user to define the bind address for each listener created with the default value being 0.0.0.0 for which the listener will receive indications from any network interface and any host/ip address.

Adds new option to pywbemlistener test to define a host name/ip address to which indications will be sent rather than just sending them to localhost.

@KSchopmeyer KSchopmeyer self-assigned this May 25, 2023
@KSchopmeyer KSchopmeyer added this to the 1.3.0 milestone May 25, 2023
@andy-maier
Copy link
Contributor

andy-maier commented May 28, 2023 •
edited
Loading

On the discussion: Given that we have quite a number of issues open for 1.3.0, I suggest backporting this whole PR.
i have set the rollback needed label.

If you agree, please rebase the PR and squash the 2 commits into one. Unless you want to split it into multiple PRs.

Also, the PR has "under work" set. If it is ready for review, please set the "review needed" label. It is still under work for the moment since I am going through the code to confirm that the default for bind-addr is really correct (WBEMListener.host="") since that is only documented in examples in BaseHTTP class.

@KSchopmeyer KSchopmeyer force-pushed the ks/container-indications branch 2 times, most recently from 7c356ed to f5252ed Compare June 7, 2023 18:08
@coveralls
Copy link

Coverage Status

coverage: 92.369% (+0.01%) from 92.357% when pulling f5252ed on ks/container-indications into 457b0e5 on master.

@KSchopmeyer
Copy link
Contributor Author

KSchopmeyer commented Jun 8, 2023 •
edited
Loading

Updated to commitPR. My only issue with rollback is that we are actually adding a parameter to the command line that this something we do not normally do in a minor release.

PRO:

  1. Fixes issue that disallows anything but localhost listeners.

CON:

  1. Adds functionality and a new Option in a rollback which we normally do not do.
  2. No user complaints so apparently not user issue.

I will go either way with respect to the rollback of this PR. The alternative is to break it into two and commit only the bug fix component. That is only two lines of code change so not a big issue

@andy-maier andy-maier removed discussion needed rollback needed Roll back/forward is needed labels Jun 12, 2023
@andy-maier
Copy link
Contributor

We decided to not roll it back because it goes beyond a pure bug fix.

andy-maier
andy-maier requested changes Jun 13, 2023
View reviewed changes
pywbemtools/pywbemlistener/_cmd_listener.py Outdated Show resolved Hide resolved
pywbemtools/pywbemlistener/_cmd_listener.py Outdated Show resolved Hide resolved
@andy-maier andy-maier self-requested a review August 22, 2023 05:36
andy-maier
andy-maier approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@andy-maier andy-maier left a comment

Choose a reason for hiding this comment

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

Approved, but please resolve the merge conflict.

@KSchopmeyer KSchopmeyer force-pushed the ks/container-indications branch 2 times, most recently from c3e2bec to 1c2fac9 Compare August 28, 2023 15:32
Changes to pywbemlistener to add host option to start and run.
fa9d973 
This change:

1. Removes the code that forces the WBEMListener host parameter to be
local host.

2. Adds an option to the start and run commands (--bind-addr, -b) that
allows the user to set a particular host name or IP address as the
WBEMListener host parameter with the default of "" for the parameter.
This allows the user to specify a specific host name or IP address
as the host parameter which is really the network interface to which
the listener binds.  In binding to a particular host or IP address, the
listener will only receive indications from that network interface on
the system or possibly even only from that specific host/IP address
depending on the OS.

3. Extends the tests for the new parameter and charactistics

4. Modifies the documentation to include the new options.
@KSchopmeyer
Copy link
Contributor Author

remarked review needed just to catch your attention. If you approve, ready for commit except issue with Python 2.7 test and pywbemcli does pass all those tests.

@KSchopmeyer KSchopmeyer mentioned this pull request Sep 9, 2023
Merged
@andy-maier andy-maier merged commit d35604f into master Sep 11, 2023
13 of 15 checks passed
@andy-maier andy-maier deleted the ks/container-indications branch September 11, 2023 15:44
@KSchopmeyer KSchopmeyer mentioned this pull request Oct 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-maier andy-maier andy-maier approved these changes

Assignees

@KSchopmeyer KSchopmeyer

Labels
None yet
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@KSchopmeyer @andy-maier @coveralls