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

Add pushlog feature for all branches (#51) #197

Closed
wants to merge 13 commits into from

Conversation

Cai0407
Copy link
Contributor

@Cai0407 Cai0407 commented Oct 12, 2015

extended patch for #185

@Cai0407 Cai0407 changed the title enble "Open pushlog between last and current builds" on beta, release and ESR branches #185 enble "Open pushlog between last and current builds" on beta, release and ESR branches Oct 12, 2015
@xabolcs
Copy link
Collaborator

xabolcs commented Oct 12, 2015

Thanks @Cai0407, will review today or tomorrow!

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 12, 2015

How about removing nightly.isTrunk() part from the if at line 134 instead morphing nightly.isTrunk()?

@xabolcs xabolcs changed the title enble "Open pushlog between last and current builds" on beta, release and ESR branches enable "Open pushlog between last and current builds" on beta, release and ESR branches Oct 12, 2015
1. removing nightly.isTrunk() part from the if at line 134
2. removing nightly.variables.platformversion.indexOf("*") from line 67 to  be available for minor update of release channel
@Cai0407
Copy link
Contributor Author

Cai0407 commented Oct 13, 2015

Hi @xabolcs, thanks for good suggestion. I have updated the patch.
I also removed nightly.variables.platformversion.indexOf("pre") from line 67 to be available for minor update of release channel (41.0.1 etc.).

@Cai0407 Cai0407 closed this Oct 13, 2015
@Cai0407 Cai0407 reopened this Oct 13, 2015
@xabolcs
Copy link
Collaborator

xabolcs commented Oct 13, 2015

@Cai0407, to be clear, this PR is for enable open-pushlog feature for all kind of release?

@Cai0407
Copy link
Contributor Author

Cai0407 commented Oct 13, 2015

@xabolcs, I have tested open-pushlog feature under these situations and all work fine;

Firefox

  1. "after current build" and "between last and current build"on 44 Nightly and 43 Developer Edition (mozilla-central, mozilla-aurora)
  2. "after current build" on 42.0b5 and "between last and current build" from 42.0b4 to 42.0b5 (mozilla-beta)
  3. "after current build" on 41.0.1 and "between last and current build" from 41.0 to 41.0.1 (mozilla-release)
  4. "after current build" on 38.3.0 ESR and "between last and current build" from 38.2.1 ESR to 38.3.0 ESR (mozilla-esr38)

Thunderbird

  1. "after current build" on 42.0b1 and "between last and current build" from 41.0b2 to 42.0b1 (comm-beta)
  2. "after current build" on 38.3.0 and "between last and current build" from 38.2.0 to 38.3.0 (comm-esr38)

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 13, 2015

In this case, it's good enough to remove the isTrunk() calls. No need for other touch.

Please remove the call from menuPopup function near line 297, and revert all other changes (except the other isTrunk() removal from yesterday).

@whimboo, I'm unsure about the rest. nightly.isTrunk(), nightlyApp.repository would be unused if this lands. Should @Cai0407 also remove those?

@@ -4,7 +4,7 @@

var nightlyApp = {

repository: ['mozilla-central','mozilla-aurora'],
repository: ['mozilla-central','mozilla-aurora','mozilla-beta','mozilla-release','mozilla-esr38','mozilla-esr31','mozilla-esr24','mozilla-esr17','mozilla-esr10'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything below mozilla-esr38 is unsupported. No need to get those added. Also I wonder if we should better have a centralized config file to handle updates to those settings more easily.

@whimboo
Copy link
Contributor

whimboo commented Oct 13, 2015

@whimboo, I'm unsure about the rest. nightly.isTrunk(), nightlyApp.repository would be unused if this lands. Should @Cai0407 also remove those?

If we don't need those anymore let kill them for sure.

@Cai0407
Copy link
Contributor Author

Cai0407 commented Oct 14, 2015

I have removed nightly.isTrunk() call from menuPopup function, and reverted all other changes for isTrunk in nightly.js (except isTrunk() removal from if at line 134), and removed unsupported old branches from browser.js, messenger.js and suite.js.

Can we remove isTrunk function itself (delete lines 60-70 of nightly.js)?

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 14, 2015

@Cai0407 sure we can remove isTrunk!
Please remove (now unused) nightlyApp.repository property too!

See @whimboo's answer!

Thanks!

@Cai0407
Copy link
Contributor Author

Cai0407 commented Oct 14, 2015

@xabolcs I have removed isTrunk function and nightlyApp.repository property.

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 15, 2015

Thanks!
Changes looks good.

Somebody needs to test them! For example me! :)

In the mean time, @whimboo could you help providing a commit message for this PR?

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 15, 2015

@whimboo!
Issue #51 should be referenced in the commit message.

@xabolcs xabolcs changed the title enable "Open pushlog between last and current builds" on beta, release and ESR branches Add pushlog feature to all branch (#51) Oct 15, 2015
@xabolcs xabolcs assigned xabolcs and unassigned Cai0407 Oct 15, 2015
@xabolcs
Copy link
Collaborator

xabolcs commented Oct 15, 2015

Somebody needs to test them! For example me! :)

Tested with Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150916094008 CSet: 7b1115ea78f7.

Looks good!

@whimboo whimboo assigned Cai0407 and unassigned xabolcs Oct 16, 2015
@whimboo
Copy link
Contributor

whimboo commented Oct 16, 2015

If testing showed that all is working fine I don't see any issues in getting this finally merged. @xabolcs mind doing it? I won't have the time today and will be away the next days.

@whimboo whimboo changed the title Add pushlog feature to all branch (#51) Add pushlog feature for all branches (#51) Oct 16, 2015
@whimboo
Copy link
Contributor

whimboo commented Oct 16, 2015

In the mean time, @whimboo could you help providing a commit message for this PR?

I would say lets take the summary of this PR? :)

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 17, 2015

Thanks @whimboo! :)

Will land this today.

@xabolcs
Copy link
Collaborator

xabolcs commented Oct 17, 2015

Thanks @Cai0407!

Landed as 60e1ba7.

@xabolcs xabolcs closed this Oct 17, 2015
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.

None yet

3 participants