Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

CI by Github action #304

Closed
wants to merge 2 commits into from
Closed

CI by Github action #304

wants to merge 2 commits into from

Conversation

hannesa2
Copy link
Contributor

Why not using Github action to run CI ?
Then this blackbox Bitrise is obsolete

@hannesa2
Copy link
Contributor Author

This Bitrise is pointless, The repo can't still not build with ./gradlew build 👎
This comes when you use a block box and the maintainer doesn't maintain it

@hannesa2
Copy link
Contributor Author

@davigonz
I agree, your solution manifestPlaceholders = [appAuthRedirectScheme: ''] is ugly

@davigonz
Copy link
Contributor

davigonz commented Feb 17, 2020

This Bitrise is pointless, The repo can't still not build with ./gradlew build 👎
This comes when you use a block box and the maintainer doesn't maintain it

Do not worry about Bitrise @hannesa2 , we are maintaining it 😉 . We are just executing gradlew ":owncloudComLibrary:assembleAndroidTest" ":owncloudComLibrary:assembleDebug" ":owncloudComLibrary:assembleDebugAndroidTest" ":owncloudComLibrary:assembleDebugUnitTest" ":owncloudComLibrary:assembleRelease" ":owncloudComLibrary:assembleReleaseUnitTest" on the owncloudComLibrary module and everything works fine there.

So, why ./gradlew build is failing when you execute it on the library project and not failing when doing the same on owncloudComLibrary module? Because, as you have already seen and fixed in this PR, something was missing in the sample client. To be honest, the sample client is not one of our priorities now, and it will need some attention in a near future for sure but we have more important things to think about now. We will decide what to do with it anyway.

What we can do from now on is including ./gradlew build for the library project in our Bitrise executions in addition to the commands that build the modules.

About Github action? We could consider it but Bitrise is working for us now and is not a bad solution.

@davigonz
Copy link
Contributor

davigonz commented Feb 17, 2020

@davigonz
I agree, your solution manifestPlaceholders = [appAuthRedirectScheme: ''] is ugly

Well, is not my solution. Unfortunately, there's many people dealing with the same issue when it comes to manifest place holders and multi modular apps. I got the solution from openid/AppAuth-Android#325 (is included in the comments along with the code) , one of the issues of AppAuth library.

I encourage you to include a better solution in that issue, if you come up with it, thx

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 17, 2020

On the other hand, i think that different things should be addressed in different PRs. Here:

  • You included integration with a CI.
  • You fixed a syntactic problem in the sample app, that made it not to compile. (sample app is not working and we have to check if we are interested or not on it).

when you work alone, it does not matter how do you manage the code. But keeping the order and scoping your ideas is basic in team work and collaboration, so every member of the team can see at a glance, what's going on.

Thanks for your contributions, the fix in the sample app is useful and works (syntactically), the GitHub CI integration is not something straightforward to decide. We trust in BitRise (not only the Android team, also the whole Mobile team that's including other platforms), so we have to check if switching to other one is a priority right now. But your effort is totally welcome and appreciated.

@hannesa2
Copy link
Contributor Author

hannesa2 commented Feb 17, 2020

Bitrise

pro:

  • Espresso tests seems to work (not always)
  • others ownCloud repos use it too

con:

  • black box
  • only internals can see
  • no setup history
  • not maintainable form externals .... current state has issues

Github action

pro:

  • Espresso tests seems to work
  • fast
  • the future
  • maintainable from everyone

con:

  • afford (but already done)

@jesmrec
Copy link
Collaborator

jesmrec commented Feb 17, 2020

If the problem is that you (or others) can not check logs or artifacts inside BitRise, we can have a look, i guess this will not be a problem. Let's check.

@michaelstingl
Copy link
Contributor

owncloud/android#2767 …for black box problem

@davigonz
Copy link
Contributor

davigonz commented Feb 18, 2020

owncloud/android#2767 …for black box problem

I guess we need to include this in any of the coming sprints if it's really a priority.

@davigonz davigonz mentioned this pull request Feb 18, 2020
@davigonz
Copy link
Contributor

davigonz commented Feb 18, 2020

What we can do from now on is including ./gradlew build for the library project in our Bitrise executions in addition to the commands that build the modules.

By the way, this is already done and sample client fixes will be merged via #306, so we can keep this PR just for CI by Github action topic.

@hannesa2
Copy link
Contributor Author

Why not merge it as an additional CI step.
Here at least, guys without Bitrise access can see what's wrong

@hannesa2
Copy link
Contributor Author

Btw, why Bitrise builds twice ? There seems something redundant

image

@theScrabi theScrabi closed this Oct 6, 2020
@JuancaG05 JuancaG05 deleted the Github-CI-library branch May 30, 2023 15:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants