Service integration > Nodes Repo Checks page #58

santiph · 2016-10-05T17:45:24Z

Update repositories checks based on service response
Update static API responses (nodejs)
Refactor Data module
Integrate with /api/crowbar to get the list of installed Add Ons
Display repositories list based on available Add Ons

…' of github.com:crowbar/crowbar-ui into features/upgrade7/nodes-repo-checks/service-integration

…upgrade7/nodes-repo-checks/service-integration

santiph · 2016-10-07T17:20:23Z

(2) #21 Add-On Product Repo Checks: Service Integration

…Checks

bisht-richa · 2016-10-10T13:45:38Z

assets/app/features/upgrade/controllers/upgrade7-backup.controller.spec.js

-                    bard.mockService(upgradeBackupFactory, {
-                        create: $q.reject(mockedErrorResponse)
+                    bard.mockService(crowbarBackupFactory, {
+                        create: $q.reject(mockedErrorResponse),


Remove comma at the end of the line

It should have failed the lints. Good catch, Richa

bisht-richa · 2016-10-10T14:11:18Z

Looks good :)

Itxaka

Looking good to me, some minor nitpicks

Itxaka · 2016-10-10T16:25:17Z

assets/app/data/crowbar/services/crowbar-backup.factory.js

    /* @ngInject */
-    function upgradeBackupFactory($q, $http, $filter, COMMON_API_V2_HEADERS) {
+    function crowbarBackupFactory($q, $http, $filter, COMMON_API_V2_HEADERS) {
        var factory = {
            create: createBackup,


create -> createBackup for consistency?

In the end it would be used like: crowbarBackupFactory().createBackup(). Don't you think it's redundant, considering we're already operating over a backup factory?

Yeah, a bit redundant but as the var+function below changed their names to include backup, I was thinking more about consistency between both methods crowbarBackupFactory().createBackup() and crowbarBackupFactory().getBackup()

I agree with @Itxaka here. It should be either create & download or createBackup & getBackup or downloadBackup.

Will be updated

Itxaka · 2016-10-10T16:26:07Z

assets/app/data/crowbar/services/crowbar-backup.factory.js

+         * @param  {Number} Backup Id to be downloaded
+         * @return {Promise}
+         */
+        function getBackup(id) {
            // this should never happen, caller should make sure 'id' is set
            if (angular.isUndefined(id)) {
                throw Error('downloadBackup() called without id.');


downloadBackup -> getBackup

Will be fixed

Itxaka · 2016-10-10T16:34:52Z

assets/app/data/crowbar/services/crowbar.factory.js

+            var requestOptions = {
+                method: 'GET',
+                url: '/api/crowbar',
+                headers: COMMON_API_V2_HEADERS


Cant we set $http.defaults.headers to avoid adding the headers on each function?

That should be isolated to crowbar endpoints only, so we allow different headers for different endpoints in the future (OpenStack, for instance)
Maybe as part of suseData.crowbar specific configuration... What do you think?

Sounds good, anything to avoid repetition 👍

@skazi0 Would Restangular help us here?

It would simplify things a bit. We could use "decoupled service" (https://github.com/mgonto/restangular#decoupled-restangular-service) for the /api/ prefix and pre-configure it with required headers and other options if needed like I did here: https://github.com/skazi0/yaia/blob/master/app/static/js/invoices.js#L22-L27

Something like https://github.com/mgonto/restangular#how-to-create-a-restangular-service-with-a-different-configuration-from-the-global-one

We could do it, but current implementation works fine so I'd leave migration to Restangular for the future or when we're blocked by some problem with direct $http usage.

I've created #61 to track it in the future
Thanks for the input

skazi0 · 2016-10-12T09:30:21Z

assets/app/data/crowbar/services/crowbar-backup.factory.js

    /* @ngInject */
-    function upgradeBackupFactory($q, $http, $filter, COMMON_API_V2_HEADERS) {
+    function crowbarBackupFactory($q, $http, $filter, COMMON_API_V2_HEADERS) {
        var factory = {
            create: createBackup,


I agree with @Itxaka here. It should be either create & download or createBackup & getBackup or downloadBackup.

skazi0 · 2016-10-12T09:36:17Z

assets/app/data/crowbar/services/crowbar-backup.factory.spec.js

-        bard.appModule('crowbarData.upgrade');
-        bard.inject('upgradeBackupFactory', '$q', '$httpBackend');
+        bard.appModule('suseData.crowbar');
+        bard.inject('crowbarBackupFactory', '$q', '$httpBackend', 'COMMON_API_V2_HEADERS');


Do we really need to inject COMMON_API_V2_HEADERS here?

According to the API documentation, we do

Santiago Petrone added 3 commits September 30, 2016 12:21

Basic Service Integration

dfb2555

Integrate with /api/upgrade/repocheck real endpoint

d9e5b34

Integrate with /api/upgrade/repocheck real endpoint

6b189f3

santiph added do not merge yet wip labels Oct 5, 2016

santiph assigned skazi0 and bisht-richa Oct 5, 2016

Santiago Petrone added 2 commits October 5, 2016 19:46

Merge branch 'features/upgrade7/nodes-repo-checks/service-integration…

7bd1a63

…' of github.com:crowbar/crowbar-ui into features/upgrade7/nodes-repo-checks/service-integration

Merge branch 'master' of github.com:crowbar/crowbar-ui into features/…

5f1a511

…upgrade7/nodes-repo-checks/service-integration

Refactor Data module, Update static responses, Test Nodes Repository …

56b4bc5

…Checks

santiph changed the title ~~Nodes Repo Checks > Service integration~~ Service integration > Nodes Repo Checks page Oct 9, 2016

bisht-richa reviewed Oct 10, 2016

View reviewed changes

Itxaka reviewed Oct 11, 2016

View reviewed changes

Santiago Petrone added 2 commits October 11, 2016 14:40

Address comments from PR

cc48972

Show repositories for AddOn products only if they're available

272b9de

santiph force-pushed the features/upgrade7/nodes-repo-checks/service-integration branch from c842ab8 to 272b9de Compare October 11, 2016 14:22

skazi0 requested changes Oct 12, 2016

View reviewed changes

Align name of Backup factory methods

409d6cb

santiph merged commit 7cc632f into master Oct 12, 2016

santiph removed do not merge yet wip labels Oct 12, 2016

skazi0 deleted the features/upgrade7/nodes-repo-checks/service-integration branch June 16, 2017 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service integration > Nodes Repo Checks page #58

Service integration > Nodes Repo Checks page #58

santiph commented Oct 5, 2016 •

edited

Loading

santiph commented Oct 7, 2016

bisht-richa Oct 10, 2016

santiph Oct 11, 2016

bisht-richa commented Oct 10, 2016

Itxaka left a comment

Itxaka Oct 10, 2016

santiph Oct 11, 2016

Itxaka Oct 11, 2016

skazi0 Oct 12, 2016

santiph Oct 12, 2016

Itxaka Oct 10, 2016

santiph Oct 11, 2016

Itxaka Oct 10, 2016

santiph Oct 11, 2016

Itxaka Oct 11, 2016

santiph Oct 11, 2016

skazi0 Oct 12, 2016

santiph Oct 12, 2016

skazi0 Oct 12, 2016

santiph Oct 12, 2016

skazi0 Oct 12, 2016

skazi0 Oct 12, 2016

santiph Oct 12, 2016

Service integration > Nodes Repo Checks page #58

Service integration > Nodes Repo Checks page #58

Conversation

santiph commented Oct 5, 2016 • edited Loading

santiph commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisht-richa commented Oct 10, 2016

Itxaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

santiph commented Oct 5, 2016 •

edited

Loading