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

feat(2437): Support read-only SCM [4] #41

Merged
merged 7 commits into from
Jun 11, 2021
Merged

feat(2437): Support read-only SCM [4] #41

merged 7 commits into from
Jun 11, 2021

Conversation

tkyi
Copy link
Member

@tkyi tkyi commented May 14, 2021

Context

Should support read-only SCM.

Objective

This PR:

  • adds validation for readOnly config in SCM options
  • takes in proper cloneType for getCheckoutCommand()
  • fixes addPRComment() and other PR comment methods

References

Related screwdriver-cd/screwdriver#2437

License

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

index.js Outdated

return { comments: prComments.body };
} catch (err) {
logger.warn('Failed to fetch PR comments: ', prComments.body.message
Copy link
Contributor

Choose a reason for hiding this comment

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

If an exception occurs, prComments is undefined and prComments.body.message will cause error

Copy link
Contributor

Choose a reason for hiding this comment

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

May be a better log would be with repoId and prNum

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

index.js Outdated
* Edit a particular comment in the PR
* @async editPrComment
* @param {Integer} commentId The id of the particular comment to be edited
* @param {Object} scmInfo The information regarding SCM like repo, owner
Copy link
Contributor

Choose a reason for hiding this comment

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

remove scmInfo (not used) and add repoId

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add prNum to param definition

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@@ -790,29 +871,57 @@ class GitlabScm extends Scm {
* @param {String} config.token The token used to authenticate to the SCM
* @return {Promise}
*/
async _addPrComment({ comment, prNum, scmUri, token }) {
async _addPrComment({ comment, prNum, scmUri }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Update jsdoc

}
});

return pullRequestComment;
Copy link
Contributor

Choose a reason for hiding this comment

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

return pullRequestComment && pullRequestComment.body

Copy link
Member Author

Choose a reason for hiding this comment

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

this change is causing some issues with tests, so leaving as is

index.js Outdated
Comment on lines 889 to 891
commentId: `${pullRequestComment.body.id}`,
createTime: `${pullRequestComment.body.created_at}`,
username: pullRequestComment.body.author.username
Copy link
Contributor

Choose a reason for hiding this comment

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

Update after Line 855 and check for undefined.

Comment on lines +902 to +913
const pullRequestComment = await this.breaker.runCommand({
json: true,
method: 'POST',
auth: {
bearer: this.config.commentUserToken
},
url: `${this.config.gitlabProtocol}://${this.config.gitlabHost}/api/v4` +
`/projects/${repoId}/merge_requests/${prNum}/notes`,
qs: {
body: comment
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

if it is convenient, refactor the api calls to a single func with url as param

Copy link
Member Author

Choose a reason for hiding this comment

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

will update at a different time

@tkyi tkyi merged commit 054142a into master Jun 11, 2021
@tkyi tkyi deleted the scmRO branch June 11, 2021 21:15
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.

2 participants