Skip to content

mgr/cephadm: Introduce tox and autopep8#36348

Merged
sebastian-philipp merged 1 commit intoceph:masterfrom
votdev:cephadm_tox_pylint
Aug 1, 2020
Merged

mgr/cephadm: Introduce tox and autopep8#36348
sebastian-philipp merged 1 commit intoceph:masterfrom
votdev:cephadm_tox_pylint

Conversation

@votdev
Copy link
Member

@votdev votdev commented Jul 29, 2020 •
edited
Loading

This PR introduces tox and autopep8 to format the Python code according to PEP8. It does NOT include any lint related things.

The Python code of the cephadm mgr module can be formated using tox -e fix from the src/pybind/mgrdirectory. This should be done by every developer before commiting a PR. Thus the code formatting keeps consistent.

Superseds: #36325

Signed-off-by: Volker Theile vtheile@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 dashboard backend
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@votdev votdev requested a review from a team as a code owner July 29, 2020 12:14
@votdev votdev force-pushed the cephadm_tox_pylint branch 2 times, most recently from 94226ff to 08c43ee Compare July 29, 2020 12:21
@votdev votdev requested a review from a team July 29, 2020 12:21
@votdev votdev requested a review from a team as a code owner July 29, 2020 12:21
@votdev votdev force-pushed the cephadm_tox_pylint branch 3 times, most recently from def779c to 483bdd1 Compare July 29, 2020 13:13
sebastian-philipp
sebastian-philipp suggested changes Jul 29, 2020
View reviewed changes
src/pybind/mgr/cephadm/tests/test_cephadm.py Outdated
])
))
#@mock.patch("mgr_module.MgrModule._ceph_get")
# @mock.patch("mgr_module.MgrModule._ceph_get")
Copy link
Contributor

@sebastian-philipp sebastian-philipp Jul 29, 2020

Choose a reason for hiding this comment

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

let's wait, till #36286 is merged

jmolmo
jmolmo approved these changes Jul 29, 2020
View reviewed changes
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.

Great thing!! Why do not include also a code linter?

@votdev
Copy link
Member Author

votdev commented Jul 29, 2020

Great thing!! Why do not include also a code linter?

Because i don't know how the cephadm code is checked in Teuthology. Adding a linter is the job of a cephadm team member with more insight than myself.

@votdev votdev force-pushed the cephadm_tox_pylint branch 2 times, most recently from 360cfb9 to b815d1e Compare July 29, 2020 16:01
@votdev
Copy link
Member Author

votdev commented Jul 30, 2020

jenkins test make check

@votdev votdev mentioned this pull request Jul 30, 2020
Closed
3 tasks
@votdev votdev force-pushed the cephadm_tox_pylint branch from b815d1e to 1945506 Compare July 30, 2020 07:28
sebastian-philipp
sebastian-philipp reviewed Jul 30, 2020
View reviewed changes
@votdev votdev force-pushed the cephadm_tox_pylint branch 2 times, most recently from a0a60e2 to f94e76a Compare July 30, 2020 09:18
@votdev votdev force-pushed the cephadm_tox_pylint branch from f94e76a to 6824123 Compare July 30, 2020 10:32
@votdev votdev force-pushed the cephadm_tox_pylint branch from 6824123 to 4ef251c Compare July 30, 2020 10:35
@votdev
Copy link
Member Author

votdev commented Jul 30, 2020

jenkins test make check

@sebastian-philipp sebastian-philipp added the wip-swagner-testing My Teuthology tests label Jul 30, 2020
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Jul 30, 2020

I think we need to split this into smaller PRs. otherwise we'll end up rebasing like forever,

@votdev
Copy link
Member Author

votdev commented Jul 30, 2020

I think we need to split this into smaller PRs. otherwise we'll end up rebasing like forever,

I'm open for suggestions.

@votdev votdev force-pushed the cephadm_tox_pylint branch from 4ef251c to 321898e Compare July 30, 2020 15:04
@sebastian-philipp
Copy link
Contributor

sebastian-philipp commented Jul 31, 2020

I think we need to split this into smaller PRs. otherwise we'll end up rebasing like forever,

I'm open for suggestions.

can you make a new PR with just the tox changes?

@votdev
mgr/cephadm: Introduce tox and autopep8
7d8a3b6 
This PR introduces tox and autopep8 to format the Python code according to PEP8. It does NOT include any lint related things.

Signed-off-by: Volker Theile <vtheile@suse.com>
@votdev votdev force-pushed the cephadm_tox_pylint branch from 321898e to 7d8a3b6 Compare July 31, 2020 12:35
@votdev votdev added the skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology label Jul 31, 2020
@votdev
Copy link
Member Author

votdev commented Jul 31, 2020

I think we need to split this into smaller PRs. otherwise we'll end up rebasing like forever,

I'm open for suggestions.

can you make a new PR with just the tox changes?

Done

@sebastian-philipp sebastian-philipp merged commit bd0b900 into ceph:master Aug 1, 2020
@tchaikov
Copy link
Contributor

tchaikov commented Aug 2, 2020

@votdev @sebastian-philipp this change broke the rpm builds. see https://shaman.ceph.com/builds/ceph/master/bd0b900325335ade70286ca77943e361aea17f78/default/221061/.

how did you test it?

@votdev votdev deleted the cephadm_tox_pylint branch August 3, 2020 07:24
@sebastian-philipp sebastian-philipp mentioned this pull request Aug 4, 2020
Merged
@mgfritch mgfritch mentioned this pull request Aug 7, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmolmo jmolmo jmolmo approved these changes

+1 more reviewer

@sebastian-philipp sebastian-philipp sebastian-philipp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm cleanup skip-teuthology For PRs whose changes do not have an effect on QA runs/changes are not being tested in Teuthology

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@votdev @sebastian-philipp @tchaikov @jmolmo