Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Multiple channels release with different installed locations and user dir #10552

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Aug 17, 2017

Todo:

Needs new brave_installer.*, background.png, brave_splash_installing.gif for each channels from @bradleyrichter
Change package.json for electron-packager, electron-installer-debian, electron-installer-redhat and electron-squirrel-startup once they are merged
Change "iconUrl: https://raw.githubusercontent.com/brave/browser-laptop/coexisted-channels/res/${channel}/app.ico, " to master once reviewers apporved

fixes #10189

If 11104 is merged:

@Auditors: @bsclifton, @bridiver, @bbondy, @aekeus

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@codecov-io
Copy link

codecov-io commented Aug 17, 2017

Codecov Report

Merging #10552 into master will decrease coverage by 0.75%.
The diff coverage is 30%.

@@            Coverage Diff             @@
##           master   #10552      +/-   ##
==========================================
- Coverage   52.74%   51.99%   -0.76%     
==========================================
  Files         269      270       +1     
  Lines       25670    25556     -114     
  Branches     4099     4074      -25     
==========================================
- Hits        13540    13288     -252     
- Misses      12130    12268     +138
Flag Coverage Δ
#unittest 51.99% <30%> (-0.76%) ⬇️
Impacted Files Coverage Δ
app/locale.js 35.23% <ø> (ø) ⬆️
js/stores/appStore.js 16.03% <0%> (+0.06%) ⬆️
app/buildConfig.js 100% <100%> (ø)
js/constants/appConfig.js 100% <100%> (ø) ⬆️
app/channel.js 50% <14.28%> (-45%) ⬇️
app/sessionStore.js 76.16% <50%> (+0.03%) ⬆️
app/common/state/ledgerState.js 45.56% <0%> (-12.91%) ⬇️
app/browser/reducers/spellCheckerReducer.js 67.44% <0%> (-11.02%) ⬇️
app/browser/api/ledger.js 30.83% <0%> (-8.97%) ⬇️
js/actions/appActions.js 16.51% <0%> (-1.81%) ⬇️
... and 13 more

appName = 'Brave'
break
default:
throw new Error('CHANNEL environment variable must be set to developer, beta or dev')
Copy link
Member

Choose a reason for hiding this comment

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

This should include nightly as well


var channels = { nightly: true, developer: true, beta: true, dev: true }
if (!channels[channel]) {
throw new Error('CHANNEL environment variable must be set to developer, beta or dev')
Copy link
Member

Choose a reason for hiding this comment

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

nightly should be here as well

appName = 'Brave'
break
default:
throw new Error('CHANNEL environment variable must be set to developer, beta or dev')
Copy link
Member

Choose a reason for hiding this comment

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

nightly should be here

@bsclifton
Copy link
Member

Besides the Muon PR, we'll also need some new images from @bradleyrichter in order to complete this one

@bridiver
Copy link
Collaborator

we should really integrate this into b-l-b which also has the brave icons, etc...

const Channel = require('./channel')
let appUserModelId = 'com.squirrel.brave.Brave'
switch (Channel.channel()) {
case 'nightly':
Copy link
Member

@bsclifton bsclifton Aug 29, 2017

Choose a reason for hiding this comment

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

kind of a lame nit-pick; you could store the text representation of the channels in an enum-like format in channel.js:

const channelStrings = {
  RELEASE: 'dev',
  BETA: 'beta',
  // ..
}

and then export / import from this file. Will make updating the constants easier 😄

switch (Channel.channel()) {
  case Channel.name.NIGHTLY:
}

@bsclifton
Copy link
Member

@darkdh can we get a rebase on this one please? 😄

@darkdh darkdh force-pushed the coexisted-channels branch from 31f714f to 3a87cba Compare September 6, 2017 19:49
@darkdh
Copy link
Member Author

darkdh commented Sep 6, 2017

rebased

@luixxiul
Copy link
Contributor

luixxiul commented Sep 7, 2017

@bradleyrichter would you tell me if the images are ready? #10552 (comment)

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Sep 8, 2017 via email

@bsclifton
Copy link
Member

One thing that's a little confusing- if I run the different binaries after building, each of them will ask if I want to make this the default browser. How do Firefox and Chrome handle this? Do their nightly/developer versions still prompt for the default browser setting on launch?

@darkdh
Copy link
Member Author

darkdh commented Sep 8, 2017

If I turn Always check default browser on Firefox and Firefox Nightly and set default browser to any of two, the other will always ask if you want to set me as default.
This is expected, some users will want to use Nightly as default browser. And different channels should be treated as different browsers

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Sep 8, 2017 via email

@bsclifton
Copy link
Member

@bradleyrichter just to confirm: we'd want to not prompt by default unless the build is in release channel?

@darkdh would it be easy to make this change? 😄

@bradleyrichter
Copy link
Contributor

bradleyrichter commented Sep 8, 2017 via email

const channel = process.env.CHANNEL

var channels = { nightly: true, developer: true, beta: true, dev: true }
if (!channels[channel]) {
Copy link
Member

@bsclifton bsclifton Sep 8, 2017

Choose a reason for hiding this comment

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

This could be changed to import the channels directly from app/channel.js.
const channels = require('../app/channel').channels

If that is an Array:
if (!channels.includes(channel)) {

if that is a Set:
if (!channels.has(channel)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems weird to me to pull dependency from app folder for tools

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's true- maybe it's fine as-is 😄


const channel = env.CHANNEL

var channels = { nightly: true, developer: true, beta: true, dev: true }
Copy link
Member

Choose a reason for hiding this comment

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

same feedback as above

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Comments left- looks great 😄 👍 I tested building and launching each type- works as expected

(will give approval once we have icons / dependencies are merged / feedback is addressed if needed)

@bbondy bbondy modified the milestones: 0.19.x (Beta Channel), 0.20.x (Developer Channel) Sep 20, 2017
@bbondy bbondy modified the milestones: 0.20.x (Beta Channel), Backlog Oct 27, 2017
@bbondy bbondy removed the sprint/1 label Oct 27, 2017
@cezaraugusto
Copy link
Contributor

@darkdh going to close this one for now given its wip for a while. Please re-open as needed. Thanks!

@bsclifton
Copy link
Member

Re-opening as we'll be revisiting this one soon! I'll remove the WIP tag since the remaining items are called out, I believe

@bsclifton
Copy link
Member

@darkdh ready for a rebase when you get a chance 😄

@darkdh
Copy link
Member Author

darkdh commented Oct 29, 2017

rebased

@darkdh darkdh force-pushed the coexisted-channels branch from 65d19cc to da9851e Compare November 3, 2017 21:12
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

I have done a great deal of testing with this- everything is working as expected 😄

Great job on this one! We can finally get true beta/nightlies out now

@bsclifton bsclifton merged commit c83e123 into master Nov 3, 2017
@bsclifton bsclifton deleted the coexisted-channels branch November 3, 2017 21:19
@bsclifton
Copy link
Member

bsclifton commented Nov 3, 2017

master c83e123
0.21.x: 47f48f3
0.20.x: cf6a8b5

darkdh pushed a commit that referenced this pull request Nov 3, 2017
Multiple channels release with different installed locations and user dir
darkdh pushed a commit that referenced this pull request Nov 3, 2017
Multiple channels release with different installed locations and user dir
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different channels should have different icons and install paths
9 participants