Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Aerogear 2506 - Spike #210

Closed
wants to merge 3 commits into from

Conversation

ciaranRoche
Copy link
Member

@ciaranRoche ciaranRoche commented Apr 30, 2018

Motivation

JIRA: https://issues.jboss.org/browse/AEROGEAR-2506

Currently, we have an example application as part of our SDK along with a separate security template app. The idea of this Spike is to consolidate these two apps together with minimum disruption to the current development flow.

Description

A suggested approach was to use mavenLocal, this seems unfeasible as a developer would be required to run ./gradlew install and rebuild the project to test every change made.

A solution I found was to include separate gradle profile files in the showcase app. One profile would link dependencies to a release version of the SDK whereas the other profile would follow the conventions we already have in the example app.

Intended Usage

Run the following command from the SDK root directory:

./scripts/setupDevelopmentShowcase.sh

This script creates an example directory, clones the showcase app, and copies the contents of the showcase's app directory into the example directory. Since we have the gradle property profile set to development in the SDK the workflow is now the same as it was before, we can make changes to the example app to test features in the SDK.

In order to commit changes to the showcase app, run the following command in the SDK root directory:

./scripts/copyShowcaseChanges.sh

This will copy changes from the example dir to app dir in the showcase template. If we simply navigate to android-showcase-template and run git status we can see all uncommitted changes to the repo, and from here create a PR against the development branch of the showcase-template app.

This method should work well if we follow the conventions from the cordova showcase application which were discussed here.

Additional Information

The approach taken, adds two gradle profiles to the showcase application. The gradle.build file in the showcase app then depends on the gradle property profile by setting the profile to production the showcase app will look to a release of the SDK, where as if profile is set to development the showcase app will look for a local version of the SDK.
Intended usage will only involve setting up your development environment one, example and android-showcase-application will be added to the .gitignore and will not be tracked for contributions to the SDK. Contributions to the showcase app can be made by running the copy script and then navigating to the showcase dir and creating the PR against the showcase app

dependencies {
implementation project(path: ":core")
implementation project(path: ":auth")
implementation project(path: ":push")
Copy link
Contributor

Choose a reason for hiding this comment

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

if the app is move to a different repo, how the references here would look like?

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 app should use the production-profile.gradle file as standard as it references a release version of the SDK, it is only for development would we use the development-profile.gradle once it has been cloned into the SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, gotcha. So there should be a script to copy the app from its own repo to the SDK's repo (or symlink). Is that what you have in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially no, I was thinking of manually cloning the showcase into the SDK for testing locally, but it would make a lot more sense that if we are following this approach to write a script to clone and copy the showcase for us, and set the gradle property profile to development

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that the script should be there to make the setup for development as easy as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, Ill get on that now, and will sync up with @TommyJ1994 later on testing it with the new showcase app

@wtrocki
Copy link
Contributor

wtrocki commented Apr 30, 2018

Really like this change. This will allow us to have smooth transition to separate showcase app without affecting contributions.

To make this mergeable I will suggest to:

  • Add script that will clone example app
  • Update documentation (contribution) to show that example app can be used for tests and point to scripts.
  • Mention in documentation that profiles are used and that production is default that should be used by developers.

@ciaranRoche World class work! Very well done!

@ciaranRoche
Copy link
Member Author

@wei-lee @TommyJ1994 @wtrocki I have included two scripts in latest commit, I also updated the description and added an intended usage to the PR above explaining the workflow.
I have also added the scripts under the assumption that the showcase repo will be aerogear/android-showcase-template.
I have tested the scripts and the workflow described above locally with my own dummy example repo.

mkdir example
git clone git@github.com:aerogear/android-showcase-template.git
cd android-showcase-template
cp -a app/. ../example
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a symlink here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, once we get the content of the showcase app moved over we can test a symlink over a full copy

Copy link
Contributor

@wei-lee wei-lee left a comment

Choose a reason for hiding this comment

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

I think the overall flow described here sounds good. Let's start moving the content to the showcase app repo, verify that it will work using the released sdks, and also work with the flow described here.

Once that's done, we can remove the example app here, and update the README file to add instructions on how to do the development work.

@wtrocki
Copy link
Contributor

wtrocki commented May 14, 2018

@ciaranRoche Is that still in progress? :>
Anything I could help to get that merged?

@ciaranRoche ciaranRoche changed the title [DO NOT MERGE] - Aerogear 2506 - Spike Aerogear 2506 - Spike May 14, 2018
@ciaranRoche
Copy link
Member Author

@wtrocki Just needs to be tested against the new showcase app, this was only a spike to see if it could work so the changes need to be tested and then can be closed and or merged.

@wtrocki
Copy link
Contributor

wtrocki commented May 14, 2018

Thanks for update. I think that current Android showcase works with development versions of the SDK so this changes is really needed.

@secondsun Any comments here before we merge this?

@secondsun
Copy link
Contributor

@wtrocki It would seem to me that it would be a lot less work to create a development dependency set in the showcase app that defines dependencies to the SDK based on a path variable contained in the showcase. For "default" releases the showcase app could download from jcenter/mavencentral as normal.

@wtrocki
Copy link
Contributor

wtrocki commented May 16, 2018

@secondsun This is how we are doing things in js and also on ios.
Problem was that at the time we were doing this spike repo was not there.
This could be easily adapted to showcase repo as this was spike.
We could create separate ticket. WDYT?

FYI @danielpassos

}

dependencies {
implementation project(path: ":core")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it use also published packages?

@secondsun
Copy link
Contributor

@wtrocki That makes sense. It is basically what this PR does just in a weird way. I'll ask @ciaranRoche about it today

@wtrocki
Copy link
Contributor

wtrocki commented May 21, 2018

However work here is still valuable to apply the same patterns to showcase app. Closing PR.

@wtrocki wtrocki closed this May 21, 2018
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