Skip to content

Retooling pbench-move-results to use the RESTful PUT API instead of direct ssh #2189

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

Merged

Conversation

riya-17
Copy link
Member

@riya-17 riya-17 commented Mar 30, 2021

This PR mainly deals with using RESTful PUT API instead of ssh for sending Tarballs from agent to server. At the time of sending of tarballs, it consists of an Authorization token (generated with the help of pbench-generate-token command)which will help for authorizing the owner of the Tarballs.

It uses 'click' for the creation of a command-line interface.
Also consists of config file changes and pytests.

Sorting of the config files is taken care in PR #2190

@portante portante added Agent API Of and relating to application programming interfaces to services and functions enhancement Server labels Mar 30, 2021
@portante portante added this to the v0.71 milestone Mar 30, 2021
@riya-17 riya-17 force-pushed the pbench-move-results branch 3 times, most recently from 8ac0ed4 to e513b7d Compare March 31, 2021 10:27
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks like a good start, but there is a bunch of polishing which needs to be done.

@riya-17 riya-17 force-pushed the pbench-move-results branch from 8426f51 to e596c34 Compare April 1, 2021 10:34
@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2021

This pull request introduces 1 alert when merging e596c34 into dc48a16 - view on LGTM.com

new alerts:

  • 1 for Unused exception object

@riya-17 riya-17 force-pushed the pbench-move-results branch 3 times, most recently from db70b4a to 3147ee0 Compare April 1, 2021 11:55
@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2021

This pull request introduces 1 alert when merging 3147ee0 into ea02995 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Looking good; just want to avoid introducing a new way of handling imports without agreeing on it (see #2238).

dbutenhof
dbutenhof previously approved these changes Apr 22, 2021
ndokos
ndokos previously approved these changes Apr 22, 2021
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

LGTM

@webbnh webbnh dismissed stale reviews from ndokos and dbutenhof via 510bb37 April 22, 2021 18:31
@webbnh webbnh force-pushed the pbench-move-results branch from 21f97a8 to 510bb37 Compare April 22, 2021 18:31
@webbnh
Copy link
Member

webbnh commented Apr 22, 2021

Reordered the import's and rebased on main.

dbutenhof
dbutenhof previously approved these changes Apr 22, 2021
npalaska
npalaska previously approved these changes Apr 22, 2021
@webbnh webbnh dismissed stale reviews from npalaska and dbutenhof via a6a4ff2 April 22, 2021 22:06
@webbnh webbnh requested a review from portante April 22, 2021 22:16
@webbnh
Copy link
Member

webbnh commented Apr 22, 2021

Functional testing, such as it was, is complete. (The server says it's good, unless I upload the same file twice, in which case the tool reports the server's objection.)

So, if I can get the nod from @portante and one other person, this PR is good to squash-and-merge.

portante
portante previously approved these changes Apr 22, 2021
Copy link
Member

@portante portante left a comment

Choose a reason for hiding this comment

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

Ship it!

dbutenhof
dbutenhof previously approved these changes Apr 23, 2021
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

Leaving the big question of whether Pete wants the 12 commits squashed before hitting the Squash and merge button?

@portante
Copy link
Member

Not sure why, but it needs a rebase, so might as well rebase and squash it to one commit.

@portante portante modified the milestones: v0.72, v0.71 Apr 23, 2021
Create a new, Click-based command which uses the server's HTTP PUT request
to upload result tarballs.  This command is intended to replace the
existing pbench-copy-result-tb script.

This commit also includes latent support for a future replacement for
the pbench-make-result-tb script and subsequently the pbench-move-results
and pbench-copy-results commands.
@webbnh webbnh dismissed stale reviews from dbutenhof and portante via c6a7e0b April 23, 2021 15:40
@webbnh webbnh force-pushed the pbench-move-results branch from a6a4ff2 to c6a7e0b Compare April 23, 2021 15:40
@webbnh
Copy link
Member

webbnh commented Apr 23, 2021

Leaving the big question of whether Pete wants the 12 commits squashed before hitting the Squash and merge button?

I want that button to be useful! (And, I want to avoid the re-approval cycle between the approval and the merge which externally squashing requires.) I wanted to try it to see if it really doesn't work acceptably....

As long as I'm griping, I resent the "Update Branch" button: if I'm going to rebase my branch, then I certainly don't want the outstanding commits from main merged into it! That button should have a pull-down like the merge button does which offers the alternative of rebasing. (And, given that our repository has merge commits disabled, the "update" button shouldn't even offer the option of merging!)

Not sure why, but it needs a rebase, so might as well rebase and squash it to one commit.

Ha! 'Cause somebody merged two PRs since I last rebased -- that's why! :-D

Anyway, the squash is done, and I'm just prattling on here while I wait for the checks to run....

@portante portante merged commit 7453480 into distributed-system-analysis:main Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agent API Of and relating to application programming interfaces to services and functions enhancement Server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants