Skip to content

TSERV-907: Delete a test job#24

Merged
richamittal merged 1 commit intomasterfrom
TSERV-907-delete-testjob
Jan 13, 2021
Merged

TSERV-907: Delete a test job#24
richamittal merged 1 commit intomasterfrom
TSERV-907-delete-testjob

Conversation

@bsubba
Copy link
Copy Markdown
Contributor

@bsubba bsubba commented Jan 11, 2021

No description provided.

Copy link
Copy Markdown
Contributor

@lootic lootic left a comment

Choose a reason for hiding this comment

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

Found a few cosmetic things that can be fixed if you like.

Comment thread bin/jobs_functions.js
}

function deleteTestJob(testjobId) {
let url = config.server + '/api/v1/testjobs'+ '/' + testjobId + '/delete';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The slash can be part of the first string and doesnt need to be concateneted seperately.

'/api/v1/testjobs'+ '/' + testjobId + '/delete';
->
'/api/v1/testjobs/' + testjobId + '/delete'

Comment thread bin/jobs_functions.js

function deleteTestJob(testjobId) {
let url = config.server + '/api/v1/testjobs'+ '/' + testjobId + '/delete';
util.output('Deleting job: '+testjobId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

formatting, add space around operators

Comment thread bin/jobs_functions.js
util.output('Deleting job: '+testjobId);
request.delete(url)
.auth(config.username, config.password)
.accept('application/junit+xml')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This accept looks weird to me, I think it could be application/json, since json is what we return when there is an error. On success we don't return any data. Same goes for cancel(which I know you havent changed but I think it can be fixed too).

Comment thread bin/jobs_functions.js
if (('status' in err) && ('message' in result.body)) {
switch (err['status']) {
case 403:
util.error(err['status'] + ': ' + result.body['message']);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is does the same as the default-case. So it can be removed. Maybe we dont need the switch at all?

@richamittal richamittal merged commit 8f7763b into master Jan 13, 2021
@richamittal richamittal deleted the TSERV-907-delete-testjob branch January 13, 2021 15:27
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.

3 participants