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

cephadm: remove redundant ERROR during check-host #38995

Merged
merged 1 commit into from Mar 2, 2021

Conversation

mgfritch
Copy link
Contributor

@mgfritch mgfritch commented Jan 20, 2021

ERROR: ERROR: No time synchronization is active

Signed-off-by: Michael Fritch mfritch@suse.com

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug

Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@mgfritch mgfritch requested a review from a team as a code owner January 20, 2021 21:30
get_hostname(), args.expect_hostname))
logger.info('Hostname "%s" matches what is expected.',
args.expect_hostname)

if errors:
raise Error('\n'.join(errors))
raise Error('\nERROR: '.join(errors))
Copy link
Member

Choose a reason for hiding this comment

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

I see ok the output produced ( I do not see the effect pointed by @sebastian-philipp )

Just improve little bit using plural or singular if it is needed:

If errors:
  error_message_header = "Errors:\n" if len(errors)> 1 else "Error:\n"
  raise Error(f'{error_message_header.join(errors)})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that will still cause issues for the mgr/cephadm logic @sebastian-philipp referenced:

errors = [_i.replace("ERROR: ", "") for _i in err if _i.startswith('ERROR')]

Copy link
Member

Choose a reason for hiding this comment

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

mmm.. I see.. with this change in place.. What if we remove that line?

@sebastian-philipp
Copy link
Contributor

this is a follow-up to #38667

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@sebastian-philipp
Copy link
Contributor

lgtm.

```
ERROR: ERROR: No time synchronization is active
```

Signed-off-by: Michael Fritch <mfritch@suse.com>
@mgfritch
Copy link
Contributor Author

mgfritch commented Mar 1, 2021

ping @jmolmo ?

Copy link
Member

@jmolmo jmolmo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants