"okteto init" asking for target deployment #918

pchico83 · 2020-06-14T23:03:57Z

Signed-off-by: Pablo Chico de Guzman pchico83@gmail.com

Fixes #907

pchico83 · 2020-06-14T23:04:31Z

Still requires some more testing, but the code should be ready for review

codecov · 2020-06-14T23:05:11Z

Codecov Report

Merging #918 into master will increase coverage by 1.13%.
The diff coverage is 47.51%.

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
+ Coverage   33.54%   34.68%   +1.13%     
==========================================
  Files          67       67              
  Lines        5386     5648     +262     
==========================================
+ Hits         1807     1959     +152     
- Misses       3429     3535     +106     
- Partials      150      154       +4

Impacted Files	Coverage Δ
cmd/restart.go	`0.00% <0.00%> (ø)`
pkg/analytics/analytics.go	`15.89% <0.00%> (-0.22%)`	⬇️
pkg/k8s/deployments/crud.go	`11.28% <0.00%> (-0.30%)`	⬇️
pkg/k8s/pods/pod.go	`12.74% <0.00%> (-1.55%)`	⬇️
pkg/k8s/services/crud.go	`4.34% <0.00%> (-1.72%)`	⬇️
pkg/model/dev.go	`65.09% <ø> (ø)`
cmd/init.go	`13.15% <14.67%> (-4.29%)`	⬇️
pkg/linguist/linguist.go	`47.47% <20.00%> (-2.00%)`	⬇️
pkg/linguist/dev.go	`83.81% <90.66%> (+19.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8a485f...5da751f. Read the comment docs.

derek · 2020-06-15T03:03:04Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

rberrelleza · 2020-06-15T15:37:57Z

cmd/init.go

 				return err
 			}

-			log.Success(fmt.Sprintf("Okteto manifest (%s) created", devPath))
+			log.Success(fmt.Sprintf("Okteto manifest (%s) created. Check its content in case you need to adapt it", devPath))


Suggested change

log.Success(fmt.Sprintf("Okteto manifest (%s) created. Check its content in case you need to adapt it", devPath))

log.Success(fmt.Sprintf("Okteto manifest (%s) created", devPath))

That second sentence sounds bad, let's not have it.

rberrelleza · 2020-06-15T15:38:40Z

cmd/init.go

-			log.Info(err)
-			return fmt.Errorf("Failed to determine the language of the current directory")
-		}
+	ifAskingForDeployment := language == ""


I'd be more explicit to make the code easier to read.

Suggested change

ifAskingForDeployment := language == ""

askForDeployment := false

if language == "" {

askForDeployment = true

}

rberrelleza · 2020-06-15T15:40:29Z

cmd/init.go

+			container = d.Spec.Template.Spec.Containers[0].Name
+		}
+
+		postfix := fmt.Sprintf("Analyzing deployment '%s'...", d.Name)


put the spinner before getting the deployment to it's also shown while doing the get

rberrelleza · 2020-06-15T15:41:23Z

cmd/init.go

+		postfix := fmt.Sprintf("Analyzing deployment '%s'...", d.Name)
+		spinner := utils.NewSpinner(postfix)
+		spinner.Start()
+		defer spinner.Stop()


doing defers with the spinner create odd behaviors in the UI. It's better to start the spinner, call teh function, stop the spinner, and then check for error.

rberrelleza · 2020-06-15T15:43:05Z

cmd/init.go

-	fmt.Printf("Which docker image do you want to use for your development container? [%s]: ", defaultImage)
-	_, err := fmt.Scanln(&image)
-	fmt.Println()
+func getDeployment(namespace string) (*appsv1.Deployment, string, error) {


can split this in two functions? one that gets the deployment / container, and another one that asks the user for info?

Also, you need to stop the spinner before showing text or the terminal can start behaving strangely.

rberrelleza · 2020-06-15T15:44:55Z

pkg/cmd/init/run.go

+}
+
+func setWorkDirFromPod(ctx context.Context, dev *model.Dev, pod *apiv1.Pod, container string, config *rest.Config, c *kubernetes.Clientset) *model.Dev {
+	if dev.WorkDir != "" {


why would this be set if we are in the process of generating the manifest?

it depends on the language

rberrelleza · 2020-06-15T15:46:40Z

pkg/cmd/init/run.go

+	for _, port := range ports {
+		localPort := port
+		if port <= 1024 {
+			localPort = port + 8000


also check if there's no collision with other port forwards?

rberrelleza · 2020-06-15T15:47:15Z

pkg/cmd/init/run.go

+
+}
+
+func setAnnotationsFromDeployment(dev *model.Dev, d *appsv1.Deployment) *model.Dev {


Suggested change

func setAnnotationsFromDeployment(dev *model.Dev, d *appsv1.Deployment) *model.Dev {

func setFluxAnnotationsIfNeeded(dev *model.Dev, d *appsv1.Deployment) *model.Dev {

rberrelleza · 2020-06-15T15:47:57Z

pkg/cmd/init/run.go

+		}
+		if pod.Spec.Containers[i].Resources.Limits != nil {
+			dev.Resources = model.ResourceRequirements{
+				Limits: model.ResourceList{


how about requests?

for dev, it is a good practice to leave them as they are. also, they are useless in Okteto Cloud

rberrelleza · 2020-06-15T15:48:20Z

pkg/cmd/init/run.go

+		if pod.Spec.Containers[i].Resources.Limits != nil {
+			dev.Resources = model.ResourceRequirements{
+				Limits: model.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("1"),


why are you hardcoding them instead of using the ones from the pod?

the ones on the pod are not valid for dev. thinks about a go binary image using 50m cpus.
I have set defaults we have seen in Okteto Cloud that makes sense. this also helps for the feature discoverability

rberrelleza · 2020-06-15T15:49:49Z

pkg/k8s/services/crud.go

+
+//GetPortsByPod returns the ports exposed via endpoint of a given pod
+func GetPortsByPod(p *apiv1.Pod, c *kubernetes.Clientset) ([]int, error) {
+	eList, err := c.CoreV1().Endpoints(p.Namespace).List(metav1.ListOptions{})


why endpoints instead of checking the pod directly?

ports at the pod level are only decorative. the endpoints tell exactly which ports are mapped to a kubernetes service (healthy or not).

rberrelleza

Looks good. I think that now that we do a lot of more things, we should explain them as they happen.npm init is a good example of this:

pchico83

@rberrelleza I will add info at the beginning of the init execution explaining its behavior.

pchico83 · 2020-06-16T06:35:50Z

pkg/cmd/init/run.go

+}
+
+func setWorkDirFromPod(ctx context.Context, dev *model.Dev, pod *apiv1.Pod, container string, config *rest.Config, c *kubernetes.Clientset) *model.Dev {
+	if dev.WorkDir != "" {


it depends on the language

pchico83 · 2020-06-16T06:36:44Z

pkg/cmd/init/run.go

+		}
+		if pod.Spec.Containers[i].Resources.Limits != nil {
+			dev.Resources = model.ResourceRequirements{
+				Limits: model.ResourceList{


for dev, it is a good practice to leave them as they are. also, they are useless in Okteto Cloud

pchico83 · 2020-06-16T06:38:09Z

pkg/cmd/init/run.go

+		if pod.Spec.Containers[i].Resources.Limits != nil {
+			dev.Resources = model.ResourceRequirements{
+				Limits: model.ResourceList{
+					apiv1.ResourceCPU:    resource.MustParse("1"),


the ones on the pod are not valid for dev. thinks about a go binary image using 50m cpus.
I have set defaults we have seen in Okteto Cloud that makes sense. this also helps for the feature discoverability

pchico83 · 2020-06-16T06:39:08Z

pkg/k8s/services/crud.go

+
+//GetPortsByPod returns the ports exposed via endpoint of a given pod
+func GetPortsByPod(p *apiv1.Pod, c *kubernetes.Clientset) ([]int, error) {
+	eList, err := c.CoreV1().Endpoints(p.Namespace).List(metav1.ListOptions{})


ports at the pod level are only decorative. the endpoints tell exactly which ports are mapped to a kubernetes service (healthy or not).

derek · 2020-06-16T13:11:31Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>

pchico83 requested review from rberrelleza and rlamana as code owners June 14, 2020 23:03

pchico83 force-pushed the init branch 2 times, most recently from 0523758 to 5ff69c6 Compare June 14, 2020 23:21

derek bot added the no-dco label Jun 15, 2020

pchico83 force-pushed the init branch from 3b35116 to 015730b Compare June 15, 2020 03:03

derek bot removed the no-dco label Jun 15, 2020

pchico83 force-pushed the init branch from 015730b to bf16708 Compare June 15, 2020 09:42

rberrelleza reviewed Jun 15, 2020

View reviewed changes

rberrelleza approved these changes Jun 15, 2020

View reviewed changes

pchico83 commented Jun 16, 2020

View reviewed changes

derek bot added the no-dco label Jun 16, 2020

pchico83 force-pushed the init branch from 4bbfc33 to 26e3734 Compare June 16, 2020 13:12

derek bot removed the no-dco label Jun 16, 2020

pchico83 added 2 commits June 16, 2020 17:23

"okteto init" asking for target deployment

36bdd28

Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>

Rename "okteto/desk" to "okteto/dev"

d2fd049

Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>

Address review comments

5da751f

Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>

pchico83 force-pushed the init branch from 26e3734 to 5da751f Compare June 16, 2020 15:23

pchico83 merged commit 206bce7 into master Jun 16, 2020

pchico83 deleted the init branch June 16, 2020 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"okteto init" asking for target deployment #918

"okteto init" asking for target deployment #918

pchico83 commented Jun 14, 2020

pchico83 commented Jun 14, 2020

codecov bot commented Jun 14, 2020 •

edited

derek bot commented Jun 15, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020 •

edited

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020 •

edited

pchico83 Jun 16, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020

rberrelleza Jun 15, 2020

pchico83 Jun 16, 2020

rberrelleza Jun 15, 2020

pchico83 Jun 16, 2020

rberrelleza Jun 15, 2020

pchico83 Jun 16, 2020

rberrelleza left a comment

pchico83 left a comment

pchico83 Jun 16, 2020

pchico83 Jun 16, 2020

pchico83 Jun 16, 2020

pchico83 Jun 16, 2020

derek bot commented Jun 16, 2020

	log.Success(fmt.Sprintf("Okteto manifest (%s) created. Check its content in case you need to adapt it", devPath))
	log.Success(fmt.Sprintf("Okteto manifest (%s) created", devPath))

-	ifAskingForDeployment := language == ""
+	askForDeployment := false
+	if language == "" {
+	  askForDeployment = true
+	}


		}

		func setAnnotationsFromDeployment(dev model.Dev, d appsv1.Deployment) *model.Dev {

	func setAnnotationsFromDeployment(dev model.Dev, d appsv1.Deployment) *model.Dev {
	func setFluxAnnotationsIfNeeded(dev model.Dev, d appsv1.Deployment) *model.Dev {

"okteto init" asking for target deployment #918

"okteto init" asking for target deployment #918

Conversation

pchico83 commented Jun 14, 2020

pchico83 commented Jun 14, 2020

codecov bot commented Jun 14, 2020 • edited

Codecov Report

derek bot commented Jun 15, 2020

Choose a reason for hiding this comment

rberrelleza Jun 15, 2020 • edited

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

rberrelleza Jun 15, 2020 • edited

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

rberrelleza left a comment

Choose a reason for hiding this comment

pchico83 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

derek bot commented Jun 16, 2020

codecov bot commented Jun 14, 2020 •

edited

rberrelleza Jun 15, 2020 •

edited

rberrelleza Jun 15, 2020 •

edited