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

Fixes #10040 - UI Bindings to CRUD rpm-ostree #5394

Merged
merged 1 commit into from Aug 18, 2015

Conversation

Projects
None yet
4 participants
@parthaa
Copy link
Member

commented Jul 31, 2015

Includes adding/updating/removing branches

@parthaa parthaa force-pushed the parthaa:crud-ui branch 3 times, most recently Jul 31, 2015

@daviddavis

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated




This comment has been minimized.

Copy link
@daviddavis

daviddavis Aug 3, 2015

Member

Is this enough whitespace? Maybe we could add a few more blank lines?

This comment has been minimized.

Copy link
@parthaa

parthaa Aug 4, 2015

Author Member

I thought you practiced Zen mediation where one tries to keep a "blank state" :)

@daviddavis

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

The cluster of buttons looks a little odd to me. Usually they're lined up horizontally. I might even recommend we move the button to under the branches section.

screen shot 2015-08-03 at 5 02 20 pm

@daviddavis

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
};

$scope.addBranch = function () {
$scope.branchObjects.push({'name': $scope.branch.name});

This comment has been minimized.

Copy link
@daviddavis

daviddavis Aug 3, 2015

Member

This looks really odd. Any way to defer adding the branch row until after the save is complete?

screen shot 2015-08-03 at 5 06 59 pm

This comment has been minimized.

Copy link
@parthaa

parthaa Aug 4, 2015

Author Member

I see what you are saying. What if theres an error or something right?

This comment has been minimized.

Copy link
@parthaa

parthaa Aug 4, 2015

Author Member

Will implement your suggestion

@daviddavis

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

I'm not seeing an error message when I try to add a dupe branch.

@parthaa

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

The cluster of buttons looks a little odd to me. Usually they're lined up horizontally. I might even recommend we move the button to under the branches section.

Same styling as Manage Packages and Manage Docker Images

@daviddavis

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

Same styling as Manage Packages and Manage Docker Images

Unless others disagree, I think we should probably fix those two as well. Although, I think we handle that outside this PR.

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
}
$scope.branchObjects = _.map(branches, function (branch) {
return {name: branch};
} );

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Nitpick - no need for the space between these two.

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
} );
};


This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Extra line

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
$scope.unwrap = function () {
return _.map($scope.branchObjects, function (branch) {
return branch.name;
} );

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

No need for space

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
['$scope', '$q', 'translate', 'Repository',
function ($scope, $q, translate, Repository) {

$scope.wrap = function (repository) {

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

I would expect wrap/unwrap to have similar behaviors - note that one modifies branchObjects on the scope and one returns the data (the latter, with a variable passed in seems better in the long run as you are then creating a function that does one thing and isn't tied to scope)

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
});
});

return deferred.promise;

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Since $scope.repository.$update should return a promise itself, why create your own?

@ehelms

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated
@@ -249,11 +258,37 @@ <h4 translate>Content Counts</h4>
<td translate>Docker Tags</td>
<td class="align-center">{{ repository.content_counts.docker_tag || 0 }}</td>
</tr>
<tr ng-show="repository.content_type == 'ostree'">

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Prefer ===

@ehelms

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated
</tbody>
</table>

<div class="divider" ng-if="repository.content_type == 'ostree'"/>

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Prefer ===

@ehelms

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated
<div class="divider" ng-if="repository.content_type == 'ostree'"/>
</section>

<section ng-if="repository.content_type == 'ostree'">

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Prefer ===

@ehelms

View changes

...tello/app/assets/javascripts/bastion_katello/repositories/details/views/repository-info.html Outdated
</tbody>
</table>
</section>

<section class="well" ng-if="permitted('edit_products', product) && !product.redhat">
<section class="well" ng-if="permitted('edit_products', product) && !product.redhat && repository.content_type != 'ostree'">

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Prefer !==

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated

<thead>
<tr>
<th class="row-select"><input type="checkbox"

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Maybe push this input down a line for readability.

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated
<thead>
<tr>
<th class="row-select"><input type="checkbox"
ng-model="allSelected"

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Where is allSelected defined? Generally it is a bad idea (and can cause weird behaviors) to bind ng-model to a primitive (in this case a boolean value) and not an object.

This comment has been minimized.

Copy link
@parthaa

parthaa Aug 7, 2015

Author Member

Where is allSelected defined? Generally it is a bad idea (and can cause weird behaviors) to bind ng-model to a primitive (in this case a boolean value) and not an object.

Took that from
https://github.com/Katello/katello/blob/master/engines/bastion_katello/app/assets/javascripts/bastion_katello/content-views/details/filters/views/package-filter-details.html#L39

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated
ng-disabled = "branchObjects.length === 0"
ng-hide="denied('edit_products')"/></th>
<th translate>Branch Name</th>
<th class=""></th>

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

No need for empty class

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated
<button class="btn btn-default"
ng-click="addBranch()"
ng-hide="false"
ng-disabled="false">

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Are these needed since they are set to false always?

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated
<tr ng-repeat="branchObject in branchObjects" row-select="branchObject">
<td class="row-select">
<input type="checkbox"
ng-hide="false"

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Is this needed?

@ehelms

View changes

...avascripts/bastion_katello/repositories/details/views/repository-manage-ostree-branches.html Outdated
</td>
<td>
<input class="form-control"
ng-hide="false || notReadable"

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Is this needed?

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated

$scope.updateBranch = function () {
$scope.saveBranches();
};

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

Do you need this wrapper function?

@ehelms

View changes

...tello/test/repositories/details/repository-details-manage-ostree-branches.controller.test.js Outdated
});
});

it('Expect save calls update right params', function() {

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 5, 2015

Member

s/Expect/expects and above as well

@ehelms

View changes

...bastion_katello/repositories/details/repository-details-manage-ostree-branches.controller.js Outdated
if (errors) {
errors(response);
}
});

This comment has been minimized.

Copy link
@ehelms

ehelms Aug 17, 2015

Member

In functions below, you defined success and error functions and then passed those to the call at the end which is nice for readability. I am OK either way at this point, but wanted to point that out if you wanted to adjust. ACK overall either way for me.

Fixes #10040 - UI Bindings to CRUD rpm-ostree
Includes adding/updating/removing  branches

@parthaa parthaa force-pushed the parthaa:crud-ui branch to ab73aa3 Aug 17, 2015

@ehelms

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

ACK

parthaa added a commit that referenced this pull request Aug 18, 2015

Merge pull request #5394 from parthaa/crud-ui
Fixes #10040 - UI Bindings to CRUD rpm-ostree

@parthaa parthaa merged commit 0e1d89e into Katello:ostree Aug 18, 2015

1 check passed

default Job result: SUCCESS
Details

@parthaa parthaa deleted the parthaa:crud-ui branch Aug 18, 2015

parthaa added a commit to parthaa/katello that referenced this pull request Aug 25, 2015

Refs #10040 - Ostree branch change now updates pulp
A bug had crept up in the previous PR related to Ostree CRUD where
updating branches really didnt refresh pulp. This commit fixes that
issue.
Refs commit -> ab73aa3
PR -> Katello#5394

parthaa added a commit to parthaa/katello that referenced this pull request Feb 12, 2016

Fixes #13585 - Adding Ostree functionality
Changes include

Fixes #10042 - Enable ostree repos in the CDN -
Katello#5455

Fixes #11611 - Copy over ostree branches on publish and promote -
Katello#5449

Ref #10040 - Minor fixes for the ostree repo create code -
Katello#5448

1) Added the missing auto_publish : true setting for OSTree repo
creation
2) Removed the nodes distributor for ostree since we are changing that
mechanism to a different model. (Nodes distributor is also not supported
for ostree content.)

Fixes #10066 - Support promoting ostree repos -
Katello#5447

Refs #10040 - Ostree branch change now updates pulp -
Katello#5431

Fixes #10063 - Allow publishing of ostree repos in content views -
Katello#5415

Fixes #11567 - Fix relative_path for ostree repos -
Katello#5442

Fixes #10040 - UI Bindings to CRUD rpm-ostree -
Katello#5394
Includes adding/updating/removing  branches

Fixes #10044 - UI to remove/add ostree repos to CVs -
Katello#5361
Fixes #10056 - Adding ostree sync -
Katello#5306
Refs #10062 - Allow users to add/remove ostree repos
-Katello#5337

Refs #10055 - Initial model bindings for OSTREE CRUD -
Katello#5240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.