This repository has been archived by the owner. It is now read-only.

Upgrade all FastlaneBuildRunner -> RemoteRunner #1093

Merged
merged 20 commits into from Jul 25, 2018

Conversation

Projects
None yet
4 participants
@snatchev
Contributor

snatchev commented Jul 20, 2018

Fixes: #1089

  • I have run rspec and corrected all errors
  • I have run rubocop and corrected all errors
  • I have run npm run test and corrected all errors
  • I have tested this change locally and tried to launch the server as well as access a project, and with that at least one build
  • If there is an existing issue, make sure to add Fixes ... as part of the PR body to reference the issue you're solving

snatchev added some commits Jul 9, 2018

big refactor to the remote runner
add refactor
add specs
update grpc and attempt to fix flakey specs

@snatchev snatchev requested review from fastlane/ci-backend as code owners Jul 20, 2018

@snatchev snatchev changed the base branch from master to refactor-RemoteRunner Jul 20, 2018

@KrauseFx

This is so great 👍

Show outdated Hide outdated app/features-json/api_controller.rb Outdated
Show outdated Hide outdated app/features-json/build_json_controller.rb Outdated
Show outdated Hide outdated app/features/build_runner/remote_runner.rb Outdated
@@ -44,7 +44,7 @@ def build_runner_task_queue
def find_build_runner(project_id:, sha: nil, build_number: nil)
return build_runners.find do |build_runner|
build_runner.project.id == project_id &&
(build_runner.sha == sha || build_runner.current_build.number == build_number)
(build_runner.current_build.sha == sha || build_runner.current_build.number == build_number)

This comment has been minimized.

@KrauseFx

KrauseFx Jul 20, 2018

Member

Why access .current_build directly? Do you think it's the better approach? If so, should we remove the .sha helper method in the other build_runner?

@KrauseFx

KrauseFx Jul 20, 2018

Member

Why access .current_build directly? Do you think it's the better approach? If so, should we remove the .sha helper method in the other build_runner?

This comment has been minimized.

@snatchev

snatchev Jul 20, 2018

Contributor

This was a bug. I don't believe we set the sha on the build_runner, when we prepare_build_object. This caused find_build_runner to match against ANY build runner (when searching by build_number) with the same project_id only (since build_runner.sha == nil and sha == nil)

@snatchev

snatchev Jul 20, 2018

Contributor

This was a bug. I don't believe we set the sha on the build_runner, when we prepare_build_object. This caused find_build_runner to match against ANY build runner (when searching by build_number) with the same project_id only (since build_runner.sha == nil and sha == nil)

This comment has been minimized.

@snatchev

snatchev Jul 20, 2018

Contributor

You'll notice that sha is an attribute on BuildRunner independent of the Build. It was possible for them to be out of sync.

@snatchev

snatchev Jul 20, 2018

Contributor

You'll notice that sha is an attribute on BuildRunner independent of the Build. It was possible for them to be out of sync.

@snatchev snatchev changed the title from [WIP] Upgrade all FastlaneBuildRunner -> RemoteRunner to Upgrade all FastlaneBuildRunner -> RemoteRunner Jul 23, 2018

@@ -22,6 +22,8 @@ export function fastlaneStatusToEnum(status: FastlaneStatus): BuildStatus {
return BuildStatus.FAILED;
case 'pending':
return BuildStatus.PENDING;
case 'running':
return BuildStatus.PENDING;

This comment has been minimized.

@nakhbari

nakhbari Jul 23, 2018

Contributor
@@ -36,7 +40,7 @@ export class BuildLogWebsocketService {
}
private createSocketUrl(projectId: string, buildNumber: number) {
return `${this.API_ROOT}?project_id=${projectId}&build_number=${
buildNumber}`;
const token = this.authService.token();

This comment has been minimized.

@nakhbari

nakhbari Jul 23, 2018

Contributor

Note: We're going to be removing our own login and be using GitHub's authentication. So maybe here we can send GitHub's token and ensure they have access to the Build's target repo.

I'll take this as a note while working on the GitHub OAuth stuff.

@nakhbari

nakhbari Jul 23, 2018

Contributor

Note: We're going to be removing our own login and be using GitHub's authentication. So maybe here we can send GitHub's token and ensure they have access to the Build's target repo.

I'll take this as a note while working on the GitHub OAuth stuff.

@@ -12,6 +13,7 @@ describe('BuildLogWebsocketService', () => {
providers: [
BuildLogWebsocketService,
{provide: DOCUMENT, useValue: {location: {host: 'host'}}},
{provide: AuthService, useValue: new AuthService(null)},

This comment has been minimized.

@nakhbari

nakhbari Jul 23, 2018

Contributor

It's best if you mock out the .token method of the authService and use that as the value instead.

ex. https://github.com/fastlane/ci/blob/master/web/app/login/login.component.spec.ts#L28

@nakhbari

nakhbari Jul 23, 2018

Contributor

It's best if you mock out the .token method of the authService and use that as the value instead.

ex. https://github.com/fastlane/ci/blob/master/web/app/login/login.component.spec.ts#L28

@snatchev snatchev changed the base branch from refactor-RemoteRunner to master Jul 25, 2018

@snatchev snatchev merged commit db06b98 into master Jul 25, 2018

4 checks passed

cla/google All necessary CLAs are signed
coverage/coveralls Coverage remained the same at 69.54%
Details
fastlane.ci: All rspecs pass All green
Details
fastlane.ci: All rubocops pass All green
Details

@snatchev snatchev deleted the upgrade-FastlaneBuildRunner branch Jul 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.