Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Notify the user that a commit has finished. #578

Merged
merged 5 commits into from Sep 24, 2014

Conversation

makyo
Copy link
Contributor

@makyo makyo commented Sep 23, 2014

No description provided.

@makyo
Copy link
Contributor Author

makyo commented Sep 23, 2014

QA: deploying changes will produce a notification once the changes have been committed to juju - this will take place after acks on a real env.

notifyCommitFinished: function() {
var db = this.get('db');
db.notifications.add(new Y.juju.models.Notification({
title: 'All changes committed',
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 id this with the changeset number? Changes completed commit: #3

@hatched
Copy link
Contributor

hatched commented Sep 23, 2014

👍 with the requested changes. This will add a nice little touch to the UI

@jujugui
Copy link
Contributor

jujugui commented Sep 23, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1923/

@jujugui
Copy link
Contributor

jujugui commented Sep 24, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1924/

@jujugui
Copy link
Contributor

jujugui commented Sep 24, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1925/

@mitechie
Copy link
Contributor

👍 Thanks for the updated message with the index.

@mitechie
Copy link
Contributor

I'm nervous about the tests not using the normal done() method with the Y.later stuff in there. It seems a bit like since that change we've had two failed test runs.

I think what @hatched was thinking is that the 'done()' is all that's required and there's no need to have the 'called' tracking at all. This is a bit of the reverse and seems a bit unsafe.

@makyo
Copy link
Contributor Author

makyo commented Sep 24, 2014

@mitechie ah, gotcha, just went in the wrong direction. Will fix.

@jujugui
Copy link
Contributor

jujugui commented Sep 24, 2014

Test FAILed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1936/

@jujugui
Copy link
Contributor

jujugui commented Sep 24, 2014

Test PASSed.
Refer to this link for build results: http://ci.jujugui.org:8080/job/juju-gui/1940/

@makyo
Copy link
Contributor Author

makyo commented Sep 24, 2014

Thanks, :shipit:

@jujugui
Copy link
Contributor

jujugui commented Sep 24, 2014

Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge

jujugui added a commit that referenced this pull request Sep 24, 2014
Notify the user that a commit has finished.
@jujugui jujugui merged commit 83d4889 into juju:develop Sep 24, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants