-
Notifications
You must be signed in to change notification settings - Fork 57
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
SSDS with PR info not SonarQube #614
Conversation
} else { | ||
nextPageStart = commits.nextPageStart | ||
} | ||
processCommits(token, repo, commits, file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this right, this make one API request per page until all commits have been found, and for each commit, make another API request to find any pull requests.
This sounds like a very expensive operation - which will get more expensive over time. Did you calculate the impact on the Bitbucket Server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the process. And no, right now we don't have calculated the impact, we are making test to probe it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hector, found out that we reach a limit at ~8k commits. The reason is not yet clear, since there is no error being shown. @hrcornejo @jorge-romero we should consider only including those commits that belong to the current product version. This requires keeping track of the commit SHA of the previous releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would other endpoints help us instead? E.g. going through the merged pull requests instead, and then plucking the ones we want to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelsauter seems like the endpoint offering the merged pull requests does not show the commit SHA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I think there is another endpoint that gives you "PR actions" or something like this which does show the merged commit. I would have to look it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hrcornejo @michaelsauter we should definitely investigate this.
@hrcornejo the current implementation using a CSV file as a result is only a prototype. The final solution removes the CSV file again and puts the data into a table within the SSDS document's section "2.3 Code Reviews". Please put this PR into Draft mode, adjust the title to reflect the end goal, and continue to use this branch for delivering the final solution. Thanks! |
*/ | ||
@SuppressWarnings(['JavaIoPackageAccess']) | ||
String generateSourceCodeReviewFile() { | ||
def file = new File("${steps.env.WORKSPACE}/${CSV_FILE}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use some namespace directories to differentiate different types of files. Please avoid writing this file directly into env.WORKSPACE. See MROPipelineUtil class members for examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created in new "review" folder
steps.env.WORKSPACE = tempFolder.getRoot().absolutePath | ||
logger = new LoggerStub(log) | ||
project = buildProject(logger) | ||
bitbucketServiceMock = new BitbucketServiceMock().setUp("csv").startServer(RECORD_WIREMOCK, BB_URL_TO_RECORD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the WireMock server utils in SpecHelper.groovy?
src/org/ods/orchestration/usecase/BitbucketTraceabilityUseCase.groovy
Outdated
Show resolved
Hide resolved
@hrcornejo @jorge-romero shall we "updraft" and proceed with the review? |
Updated the templates in opendevstack/ods-document-generation-templates#36 |
@hrcornejo is this ready to go? If yes, please resolve conflicts and let's get this merged. |
@metmajer solved, pending approval to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! I approve, but we need to ensure we integrate with Jira in the most efficient way. @michaelsauter has proposed an alternative REST API to be used. Let's follow up on this in the next iteration.
@hrcornejo please squash and merge into a single commit. Cheers! |
Generate a CSV file with bitbucket info (commits and PR over default branch per every repo) to use later in the generation of SSDS document.