chrishkchris commented on pull request #790: URL: https://github.com/apache/singa/pull/790#issuecomment-688666652
> > Would it be a better practice if we use the merged code instead of the PR branch for CI test? > > It is possible to remove `pull_request:` from the trigger events in the [workflow yaml files](https://github.com/apache/singa/tree/dev/.github/workflows): > > ``` > on: > push: > pull_request: > ``` > > or we can keep it as a guide for the PR reviewer but not requiring all PR to pass all the tests. It will be up to the PR reviewer to decide if the failed tests should be fixed in this PR or not. I would propose to keep it because sometimes it is better to discover the failed tests in the PR before the merge. But all the team can vote on the best options for triggering the workflow. There are many [events for workflow trigger](https://docs.github.com/en/actions/reference/events-that-trigger-workflows) and may be some of them can be enabled on some specific workflows. > > > Also another problem is that if team members A and B working on the same branch, their code can pass the CI test separately, but may not be compatible to each other (e.g. due to API change) > > If the is enabled, this problem will be discovered when the second team member pushes the code and the workflow is triggered. Thanks a lot for the opinion. I also think that keeping all the test is good, as it helps us to safeguard the code base. I suggest using the merged code (i.e. the code merging from PR branch and dev branch) for CI test instead of using PR branch code only for CI test, the flow I suggest is something like this: Step 1: Merged_code = Merge (PR branch, base branch) Step 2: Test_result = CI_Test(Merged_code) Step 3: Make decision whether to merge the code based on Test_result. Maybe this is equivalent to the `push:` trigger but I am not sure. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
