Skip to content

cephadm: remove redundant ERROR during check-host#38995

Merged
sebastian-philipp merged 1 commit intoceph:masterfrom
mgfritch:cephadm-check-host-errors
Mar 2, 2021
Merged

cephadm: remove redundant ERROR during check-host#38995
sebastian-philipp merged 1 commit intoceph:masterfrom
mgfritch:cephadm-check-host-errors

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

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>
@sebastian-philipp
Copy link
Contributor

@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

@sebastian-philipp sebastian-philipp merged commit c28524d into ceph:master Mar 2, 2021
@mgfritch mgfritch deleted the cephadm-check-host-errors branch March 2, 2021 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants