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

ducktape: Save command line on start and add it to HTML report #27

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

savex
Copy link

@savex savex commented Aug 23, 2023

With minimal changes to structure and test flow, added command line as it was in sys.argv

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2023

CLA assistant check
All committers have signed the CLA.

@savex savex requested a review from gousteris August 23, 2023 00:24
@andrewhsu
Copy link
Member

i'm seeing error in the python check

@savex
Copy link
Author

savex commented Aug 28, 2023

Python py38 check failed due to random port that it tries to use is already in use (real reason). And it is completely unrelated to this code

@ivotron
Copy link
Member

ivotron commented Aug 28, 2023

i wonder when/how this started to fail. we should create a ticket to fix. Maybe synchronizing with upstream would take care of it (?)

@savex savex changed the title ducktape: Same command line in resulst and add it to HTML report ducktape: Save command line on start and add it to HTML report Aug 30, 2023
@savex
Copy link
Author

savex commented Sep 5, 2023

/recheck

@savex
Copy link
Author

savex commented Sep 5, 2023

Rebased to master, fixed one flake8 error and bumped pyyaml version to 6.0.1 according to this issue

Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

this needs a rebase with the tip of master. looks like dd1fe9e is a merge commit. rebase will have the git commits stacked on top of master which will help reason about the differences with upstream.

EDIT: i think this PR should have 2 commits but it shows a lot more of the older ones which was a hint to me it was not rebased.

other than that, changes lgtm.

@savex
Copy link
Author

savex commented Sep 8, 2023

I personally do not know what to do next. I am lost in the branches and where should what go :)

@savex savex requested a review from andrewhsu September 8, 2023 21:19
@andrewhsu
Copy link
Member

I personally do not know what to do next. I am lost in the branches and where should what go :)

i just tried to rebase against master with

git rebase master

and it looks like git has conflict with each one of the 48 commits in this PR which i feel is unnecessary to resolve because it is pretty much just doing what was done in https://github.com/redpanda-data/devprod/issues/332 all over again.

i think it would be easier to open a new PR to master (make sure you start from latest of tip of master) and just create a new commit with the changes shown in https://github.com/redpanda-data/ducktape/pull/27/files which are only +10 -1 lines.

@savex savex force-pushed the cdt-vtools-2043-report-cmd-line branch 2 times, most recently from 259bef9 to 8117129 Compare September 8, 2023 23:08
Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

the git history for this PR is looking much better now, just 4 commits but i believe 8a6567d and 4b48071 are not needed in this PR because the changes already exist in the tree. you can drop these 2 commits with:

git rebase -i HEAD~4

@savex savex force-pushed the cdt-vtools-2043-report-cmd-line branch from 8117129 to 51ef2b8 Compare September 11, 2023 13:07
@savex
Copy link
Author

savex commented Sep 11, 2023

the git history for this PR is looking much better now, just 4 commits but i believe 8a6567d and 4b48071 are not needed in this PR because the changes already exist in the tree. you can drop these 2 commits with:

git rebase -i HEAD~4

Done

Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

after this merges, be sure to update https://github.com/redpanda-data/redpanda/blob/dev/tests/setup.py with the new git commit from master branch

@andrewhsu
Copy link
Member

after this merges, be sure to update https://github.com/redpanda-data/redpanda/blob/dev/tests/setup.py with the new git commit from master branch

or update redpanda-data/redpanda#13116

@savex savex merged commit 71a2932 into master Sep 12, 2023
2 checks passed
@savex savex deleted the cdt-vtools-2043-report-cmd-line branch September 12, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants