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

Cirrus ci for both ios and android #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hanabi1224
Copy link

Build fails for IOS tho: https://cirrus-ci.com/build/5686589493084160

To setup CI(free for OSS), go to https://github.com/marketplace/cirrus-ci and follow instructions here: https://cirrus-ci.org/guide/quick-start/

Please let me know your concerns

@tomalabaster
Copy link
Collaborator

Let's look at fixing the iOS build, otherwise all looks good. I'm hoping to have time over the weekend to finish off my Auth implementation, I'll take a look at this build then as well.

@aloisdeniel would you be happy with CI set up on this?

@hanabi1224
Copy link
Author

@tomalabaster I would prefer to have CI setup with gated PR merge first, then I can help fix IOS build

@tomalabaster
Copy link
Collaborator

Ok sure, that makes sense. I've worked out a fix for the iOS build anyway so after we merge this in I'll make a PR we can test on.

@hanabi1224
Copy link
Author

@tomalabaster I just took a look, the build failure was simply caused by outdated project template, I can send my fix in another PR

@tomalabaster
Copy link
Collaborator

I can see that we're not running tests as part of this (because there aren't any). I think we should agree to start writing basic tests (as all this is really doing is calling methods on the given platform) in the future and include testing as part of CI. To get this PR in it could be acceptable to remove the current default Flutter test so flutter test passes.

@tomalabaster
Copy link
Collaborator

@tomalabaster I just took a look, the build failure was simply caused by outdated project template, I can send my fix in another PR

Sounds like we found the same fix 👍

@hanabi1224
Copy link
Author

@tomalabaster I just used a third branch with ci+ios build fix to verify the fix: https://cirrus-ci.com/build/5689608251113472 looks like ios build now passes

@tomalabaster
Copy link
Collaborator

I've seen your test branch worked with CI 👍 . Happy to merge in once we've come to a conclusion on whether or not to run flutter test as part of CI. Personally I think we should really run flutter test, even if the tests are very basic as per my previous comment.

@hanabi1224
Copy link
Author

@tomalabaster I can only see template tests in the example project but no tests present in plugin projects, that's why I commented out test_script

@tomalabaster
Copy link
Collaborator

We should remove the template tests then. I don't have access to set up Cirrus on this repo so @aloisdeniel will need to do it.

@aloisdeniel, are you OK with having Cirrus CI set up? And if so, are you able to install it on this repo?

@aloisdeniel
Copy link
Owner

Hi, thanks for setting up all of that!

I've never worked with Cirrus CI before : can anyone explain what it does exactly ? Does it triggers a Azure DevOps build from a local version of the extension ?

CI would be greatly appreciated since extension development can be painful ...

@hanabi1224
Copy link
Author

@aloisdeniel For GitHub, it’s just like Travis with native flutter support?

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.

3 participants