Skip to content

Conversation

Draiken
Copy link
Contributor

@Draiken Draiken commented May 19, 2016

Still WIP

Not sure how to deal with the NewReplicaSet and OldReplicaSets fields. Are they really needed for the frontend?

Part of #595


This change is Reviewable

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Draiken
Copy link
Contributor Author

Draiken commented May 19, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@codecov-io
Copy link

codecov-io commented May 19, 2016

Current coverage is 95.22%

Merging #750 into master will increase coverage by 0.47%

  1. File ...ereader_directive.js (not in diff) was modified. more
    • Misses -6
    • Partials 0
    • Hits +6
@@             master       #750   diff @@
==========================================
  Files           173        173          
  Lines          1332       1338     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1262       1274    +12   
+ Misses           70         64     -6   
  Partials          0          0          

Powered by Codecov. Last updated by 3f66d1c...79aaa80

@bryk
Copy link
Contributor

bryk commented May 20, 2016

@floreks @digitalfishpond Can you review this?

@bryk
Copy link
Contributor

bryk commented May 20, 2016

Hey, this is nice! Can you make it merge-able so that we merge it and do more work in follow up PRs? We like small PRs here :)

Not sure how to deal with the NewReplicaSet and OldReplicaSets fields. Are they really needed for the frontend?

I think it is worth to show them if we can explain easily to users what they are.


Reviewed 1 of 3 files at r1, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@floreks
Copy link
Member

floreks commented May 20, 2016

Old/new replica sets are somehow related to deployment so we should show them. One approach could be to return ReplicaSetList for both from backend and display them using replicasetcardlist component with appropriate title.

@bryk what do you think?

This PR looks really nice! Just few comments.

Previously, bryk (Piotr Bryk) wrote…

@floreks @digitalfishpond Can you review this?


Reviewed 1 of 3 files at r1, 3 of 4 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


src/app/backend/resource/deployment/deploymentdetail.go, line 26 [r3] (raw file):

  // Status
  Status extensions.DeploymentStatus `json:"status"`

Please use our structures. For this you could add structure similar to PodInfo with status info. I think we could omit ObservedGeneration here. I don't see this info in kubectl describe. Do you know what's it for?


src/app/backend/resource/deployment/deploymentdetail.go, line 38 [r3] (raw file):

  // OldReplicaSets
  OldReplicaSets []extensions.ReplicaSet `json:"oldReplicaSets"`

This can be our ReplicaSet object.


src/app/backend/resource/deployment/deploymentdetail.go, line 41 [r3] (raw file):

  // New replica set used by this deployment
  NewReplicaSet extensions.ReplicaSet `json:"newReplicaSet"`

Same here


src/app/backend/resource/deployment/deploymentdetail.go, line 89 [r3] (raw file):

}

func getDeploymentDetail(deployment *extensions.Deployment, old []*extensions.ReplicaSet, newReplicaSet *extensions.ReplicaSet, events *common.EventList) *DeploymentDetail {

Could you use gofmt to format the code? Some lines are getting pretty long.


Comments from Reviewable

@bryk
Copy link
Contributor

bryk commented May 20, 2016

And yeah, only minor comments. IMO, we can merge this soon.

Previously, floreks (Sebastian Florek) wrote…

Old/new replica sets are somehow related to deployment so we should show them. One approach could be to return ReplicaSetList for both from backend and display them using replicasetcardlist component with appropriate title.

@bryk what do you think?

This PR looks really nice! Just few comments.


Review status: all files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@floreks floreks mentioned this pull request May 20, 2016
22 tasks
}
}

func toReplicaSetList(resourceList []extensions.ReplicaSet, pods []api.Pod) replicaset.ReplicaSetList {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bit of duplication here on transforming extension replicaset objects into the dashboard ones. I'm not very familiar with Go or the source code, so I wasn't sure if I should, for example, export the getReplicaSetList on the replicaset package.

@Draiken
Copy link
Contributor Author

Draiken commented May 20, 2016

I'm using gofmt and it didn't change the line sizes. Sorry I didn't know I had to do it manually.

@bryk
Copy link
Contributor

bryk commented May 23, 2016

Last cosmetic comment and I think we can merge :) @floreks Also take a look, if you can.

Previously, Draiken (Luiz Felipe G. Pereira) wrote…

I'm using gofmt and it didn't change the line sizes. Sorry I didn't know I had to do it manually.


Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/app/backend/resource/deployment/deploymentdetail.go, line 125 [r4] (raw file):

... so I wasn't sure if I should, for example, export the getReplicaSetList on the replicaset package.

Yeah, go ahead and export it here. In general, try to avoid copy & paste when possible.

To export getReplicaSetList just start it with Capital Letter.


Comments from Reviewable

@Draiken Draiken force-pushed the deployment-detail-backend branch from 9238e3f to b0758e7 Compare May 23, 2016 22:41
@bryk
Copy link
Contributor

bryk commented May 24, 2016

:lgtm: Merging. If you need any more help, talk to me or us on slack: https://kubernetes.slack.com/messages/sig-ui/

Previously, bryk (Piotr Bryk) wrote…

Last cosmetic comment and I think we can merge :) @floreks Also take a look, if you can.


Reviewed 1 of 3 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@bryk bryk merged commit cf3ed3e into kubernetes:master May 24, 2016
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.

5 participants