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

Update CreatePersistentVolumeClaimController to use getPreferredVersion #2160

Conversation

benjaminapetersen
Copy link
Contributor

No view updates.

@spadgett

@@ -24,6 +25,8 @@ angular.module('openshiftConsole')
$scope.accessModes="ReadWriteOnce";
$scope.claim = {};

var perpersistentVolumeClaimsVersion = APIService.getPreferredVersion('buildconfigs');
Copy link
Member

Choose a reason for hiding this comment

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

s/buildconfigs/persistentvolumeclaims

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx @spadgett, updating

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from 1ee46ca to 30e2807 Compare September 25, 2017 18:31
@benjaminapetersen
Copy link
Contributor Author

updated.

@@ -24,6 +25,8 @@ angular.module('openshiftConsole')
$scope.accessModes="ReadWriteOnce";
$scope.claim = {};

var perpersistentVolumeClaimsVersion = APIService.getPreferredVersion('persistentvolumeclaims');
Copy link
Member

Choose a reason for hiding this comment

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

perper? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh good grief, not cool. ok, my review process sucks. lemme fix my system, sorry ☹️

Copy link
Member

Choose a reason for hiding this comment

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

To be fair, I missed that on first review :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at least var name consistently wrong but not broken, heh.... Updating.

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from 30e2807 to b5e2aec Compare September 25, 2017 18:53
@benjaminapetersen
Copy link
Contributor Author

ok updated w/fixed var name.

spadgett
spadgett previously approved these changes Sep 25, 2017
@spadgett spadgett dismissed their stale review September 25, 2017 18:59

Version is not updated in the resource body we POST

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Hey @benjaminapetersen Missed this on first review, but we shouldn't use the preferred version for create. We should use the version in the object body that we construct. That should be updated to use the API group.

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from b5e2aec to 909e639 Compare September 25, 2017 19:46
@benjaminapetersen
Copy link
Contributor Author

Updated w/objectToResourceGroupVersion.

@@ -49,7 +52,7 @@ angular.module('openshiftConsole')
.then(_.spread(function(project, context) {
$scope.project = project;

if (!AuthorizationService.canI('persistentvolumeclaims', 'create', $routeParams.project)) {
if (!AuthorizationService.canI(persistentVolumeClaimsVersion, 'create', $routeParams.project)) {
Copy link
Member

Choose a reason for hiding this comment

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

We should be checking the same resourceGroupVersion that we create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. Let me update.

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from 909e639 to 4910f8d Compare September 26, 2017 13:51
@benjaminapetersen
Copy link
Contributor Author

updated

kind: "PersistentVolumeClaim",
apiVersion: "v1",
metadata: {
name: $scope.claim.name,
Copy link
Member

Choose a reason for hiding this comment

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

$scope.claim.name is not set at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oop, thx, fixed & tested.

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from 4910f8d to 8b7d4c4 Compare September 27, 2017 14:18
@benjaminapetersen
Copy link
Contributor Author

updated

@@ -59,7 +75,8 @@ angular.module('openshiftConsole')
if ($scope.createPersistentVolumeClaimForm.$valid) {
$scope.disableInputs = true;
var claim = generatePersistentVolumeClaim();
DataService.create('persistentvolumeclaims', null, claim, context)
var createClaimVersion = APIService.objectToResourceGroupVersion(claim);
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the createPVCVersion from above since we already have it

Copy link
Contributor Author

@benjaminapetersen benjaminapetersen Sep 27, 2017

Choose a reason for hiding this comment

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

Yeah doesn't really make sense to do the work again, should still have the same rgv value. I'll update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to move it up as well, most of these I have near the top of the file, before the ProjectService.get()

@benjaminapetersen benjaminapetersen force-pushed the bpetersen/trello/api-groups/create-pvc branch from 8b7d4c4 to eb19bc2 Compare September 27, 2017 14:58

Unverified

This user has not yet uploaded their public signing key.
@benjaminapetersen
Copy link
Contributor Author

benjaminapetersen commented Sep 27, 2017

Updated/retested.

@spadgett
Copy link
Member

[merge]

@spadgett
Copy link
Member

flake #1684

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to eb19bc2

@openshift-bot
Copy link

openshift-bot commented Sep 27, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/278/) (Base Commit: 0177053) (PR Branch Commit: eb19bc2)

@openshift-bot openshift-bot merged commit b09dafe into openshift:master Sep 27, 2017
@benjaminapetersen benjaminapetersen deleted the bpetersen/trello/api-groups/create-pvc branch September 27, 2017 18:52
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