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

Flutter Stager #416

Merged
merged 1 commit into from
Mar 9, 2017
Merged

Flutter Stager #416

merged 1 commit into from
Mar 9, 2017

Conversation

FaisalAbid
Copy link
Member

Addresses the issue flutter/flutter#8159

How it works

  • There are 10 Firebase projects setup.
  • There is a Master Firebase Project Setup with Firebase Database that coordinates everything
  • When a PR is made, we find a free firebase project and deploy it, storing what branch was deployed on which PR
  • When an update to a PR is made, we find the project that was associated with that PR and return that
  • It finds the Pull Request Issue and makes a comment from a bot account with the URL to that staging url.
  • This scales well by just adding more entries to the Firebase Database
  • We do a cleanup when it runs by checking the branches currently in the database still exist as Pull requests, if no then we delete the entry and let something else override it

Please review!

I would also like to point out the following line, would like someones thoughts on this, is there a better way to do this?
if [ "$BRANCH" != "master" ]; then

Please ask questions if something is unclear of if something needs to be documented more.

I will be providing @sethladd with the access tokens as well as project access to all the firebase projects shortly.

@googlebot googlebot added the cla: yes Contributor has signed the Contributor License Agreement label Mar 6, 2017
@FaisalAbid
Copy link
Member Author

Sample deploy url https://fir-deploy-one.firebaseapp.com

@FaisalAbid
Copy link
Member Author

FaisalAbid commented Mar 6, 2017

Fixing up a oAuth issue with the firebase token, will update PR shortly

@FaisalAbid FaisalAbid force-pushed the flutter_stager branch 2 times, most recently from c4c6d72 to 24ef4ac Compare March 6, 2017 19:14
@sethladd
Copy link
Contributor

sethladd commented Mar 6, 2017

What does "fir" mean? Just curious...

@FaisalAbid
Copy link
Member Author

@sethladd I created the projects as firebase-deploy-one and Firebase converts the name "firebase" to fir lol

version: 0.0.1
dependencies:
firebase: '3.0.0'
github: 'any'
Copy link
Contributor

Choose a reason for hiding this comment

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

please lock to a version with ^

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 do

tool/travis.sh Outdated
@@ -82,3 +84,16 @@ if [ "$TRAVIS_EVENT_TYPE" = "push" ] && [ "$TRAVIS_BRANCH" = "master" ]; then
npm install --global firebase-tools@3.0.0
firebase -P sweltering-fire-2088 --token "$FIREBASE_TOKEN" deploy
fi

if [ "$BRANCH" != "master" ]; then
echo "deploying to test environment"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "to staging environment" instead of "test environment" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

got it

@@ -0,0 +1,90 @@
// Copyright (c) 2017, FaisalAbid. All rights reserved. Use of this source code
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you run the formatter over this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have webstorm to auto format the entire file on save, i'll fix this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please update the copyright :)

Copy link
Contributor

Choose a reason for hiding this comment

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

And then please add yourself to the AUTHORS file. Which, if it doesn't exist, please add it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes ofcourse!

Copy link
Contributor

@Hixie Hixie Mar 6, 2017

Choose a reason for hiding this comment

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

The AUTHORS file exists, it's in the root of the repo. The copyright on every other file should be identical (that's what the CLA you signed is about :-) ).

Disregard, turns out we've screwed this up.

@sethladd
Copy link
Contributor

sethladd commented Mar 6, 2017

Oh, weird. Would it make more sense to use flutter-io-staging-one, etc ?

@FaisalAbid
Copy link
Member Author

Sure! just a matter of renaming the projects, I can do that easily


main(List<String> arguments) async {
String branchName = arguments[0];
Map env = Platform.environment;
Copy link
Contributor

Choose a reason for hiding this comment

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

generics Map<String, String>

return true;
}

Future<List> getPullRequests() async {
Copy link
Contributor

Choose a reason for hiding this comment

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

generics: Future<List<PullRequest>>


Future<List> getPullRequests() async {
Stream<PullRequest> prs = await github.pullRequests.list(new RepositorySlug.full("flutter/website"));
List prsList = await prs.toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

generics: List<PullRequest>

}

Future<bool> isBranchOld(String branchName) async {
List prsList = await getPullRequests();
Copy link
Contributor

Choose a reason for hiding this comment

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

generics: List<PullRequest>


FirebaseClient fbClient = new FirebaseClient(firebaseAuth);

List prsList = await getPullRequests();
Copy link
Contributor

Choose a reason for hiding this comment

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

generics: List<PullRequest>


GitHub github;
String firebaseAuth;
PullRequest currentPullrequest;
Copy link
Contributor

Choose a reason for hiding this comment

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

currentPullrequest -> currentPullRequest

currentPullrequest = p;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of for loop: currentPullRequest = prsList.firstWhere((p) => p.head.ref == branchName);

Copy link
Member Author

Choose a reason for hiding this comment

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

Love this, thanks @a14n - great idea

return false;
}
}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as: return !prsList.any((p) => p.head.ref == branchName); ?

});
}

print(projectToDeploy);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add more context to the print? Or is printing the name sufficiently useful?

Copy link
Member Author

Choose a reason for hiding this comment

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

The print value is used in travis.sh to populate firebase deploy, so it just needs to be on its own

}

Future<bool> isBranchOld(String branchName) async {
List<PullRequest> prsList = await getPullRequests();
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: do we have to use the type name in the variable name? prsList or just pullRequests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure i can rename it to pullRequests

@sethladd
Copy link
Contributor

sethladd commented Mar 8, 2017

What's the risk of landing this and living with it for a little while?

@FaisalAbid
Copy link
Member Author

No risk, GitHub flagged my Bot account since there was no activity on it, I emailed them to get it reactivated, once that is up we can merge this.

In my tests on my private Travis its worked well, staged it, sent out the comment etc.

@sethladd
Copy link
Contributor

sethladd commented Mar 8, 2017

Let's try merging this tomorrow (not at night time here :) and seeing how it works.

@FaisalAbid
Copy link
Member Author

Great, Github will have replied back to me by then too. Excited!

@FlutterBot
Copy link

Test comment

@sethladd
Copy link
Contributor

sethladd commented Mar 8, 2017

Let me know when we can merge this. I see Travis is red, not sure that's expected.

@FaisalAbid
Copy link
Member Author

Travis is red because of flutter/flutter#8646

@FlutterBot
Copy link

Staging URL Generated At https://flutter-io-deploy-one.firebaseapp.com . Please allow Travis Build to finish to view the URL.

@FaisalAbid
Copy link
Member Author

@sethladd yay worked, flutter-bot even wrote the comment!

@sethladd
Copy link
Contributor

sethladd commented Mar 9, 2017

Wow, very cool! Once Travis goes all green, we can merge and watch what happens.

FYI @LarkAscending @Sfshaza

@sethladd sethladd merged commit 7703bfb into master Mar 9, 2017
@sethladd sethladd deleted the flutter_stager branch March 9, 2017 23:15
@LarkAscending
Copy link
Contributor

LarkAscending commented Mar 10, 2017

Travis website builds are failing for #431 and #432.

log

Error: Command requires authentication, please run firebase login

upstream error:

Unauthorized request.
#0      FirebaseClient.send (package:firebase/firebase_io.dart:95:11)

Could this be a dependency issue? Neither @cbracken nor @LarkAscending has the dependencies installed for FB CLI.

@FaisalAbid
Copy link
Member Author

Thats weird that it's seeing a Google oauth issue, im investigating why

@LarkAscending
Copy link
Contributor

In case it's helpful, Firebase CLI setup.
Scroll to the last category on the page - auth:import requires it.

@FaisalAbid
Copy link
Member Author

Okay so the update on this, its failing actually because GitHub is saying too many requests from the travis ip. we are using the authenticated token. I am investigating the issue right now

@LarkAscending
Copy link
Contributor

These PRs are failing again with the auth issue.
Totally looking forward to using this feature...

/cc @cbracken

@sethladd
Copy link
Contributor

sethladd commented Mar 11, 2017 via email

@FaisalAbid
Copy link
Member Author

FaisalAbid commented Mar 11, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants