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

status server uris sync up #5748

Merged
merged 1 commit into from
Oct 5, 2023
Merged

status server uris sync up #5748

merged 1 commit into from
Oct 5, 2023

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Aug 11, 2023

When user sets only one of the status server values (uri, listen) Then the avocado will stop working because it will listen on different uri than the messages will be sent. This change will synchronize these values if the user sets only one of them.

Reference: #5740

@richtja richtja added the bug label Aug 11, 2023
@richtja richtja requested a review from clebergnu August 11, 2023 09:58
@richtja richtja self-assigned this Aug 11, 2023
@mr-avocado
Copy link

mr-avocado bot commented Aug 11, 2023

Dear contributor,
Avocado is currently under sprint #103, which is due to release an LTS (Long Term Stability) release.
Please avoid merging changes that do not fall into these categories:

  • Bug fixes
  • Usability Improvements
  • Documentation updates

As for the Avocado utility modules (“avocado.utils”) it is OK to introduce new functionality,
but changes to the existing APIs (including interface and behavior) should be avoided.
These kind of changes should wait until sprint #104.

clebergnu
clebergnu requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

This looks good, but I'd like to request:

  1. a functional test (so that we don't hit this again)
  2. changes to the help message for these configurations, so that users are aware that values can be set on their behalf

@richtja
Copy link
Contributor Author

richtja commented Aug 28, 2023

This looks good, but I'd like to request:

1. a functional test (so that we don't hit this again)

2. changes to the help message for these configurations, so that users are aware that values can be set on their behalf

Hi @clebergnu, thanks for the review. I will address your proposals.

@richtja richtja requested a review from clebergnu August 28, 2023 14:45
@richtja richtja linked an issue Oct 4, 2023 that may be closed by this pull request
Status server is not disabled on --nrunner-status-server-disable-auto and --nrunner-status-server-uri #5740
Closed
clebergnu
clebergnu requested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja , there's just one issue with the help message and we should be good here!

avocado/plugins/runner_nrunner.py Outdated Show resolved Hide resolved
@richtja
status server uris sync up
c0eb3cb 
When user sets only one of the status server values (uri, listen) Then
the avocado will stop working because it will listen on different uri
than the messages will be sent. This change will synchronize these
values if the user sets only one of them.

Reference: avocado-framework#5740
Signed-off-by: Jan Richter <jarichte@redhat.com>
@richtja richtja requested a review from clebergnu October 5, 2023 09:23
clebergnu
clebergnu approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@clebergnu clebergnu merged commit 649521b into avocado-framework:master Oct 5, 2023
@richtja richtja deleted the status_seerver branch October 9, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clebergnu clebergnu clebergnu approved these changes

Assignees

@richtja richtja

Labels
bug
Projects
Default project
Archived in project
Milestone
#103 (LTS release - Sound of Freedom)
Development

Successfully merging this pull request may close these issues.

Status server is not disabled on --nrunner-status-server-disable-auto and --nrunner-status-server-uri
2 participants
@richtja @clebergnu