Add autocreate field to manifest #1289

jLopezbarb · 2021-02-18T13:31:29Z

Signed-off-by: jLopezbarb javier@okteto.com

Proposed changes

Creates a autocrate field in manifest to deploy even when there is no application deployed on Okteto Cloud.
Returns an error if someone tries to create a deployment without adding this field on manifest or adding the --deploy arg

Signed-off-by: jLopezbarb <javier@okteto.com>

pchico83 · 2021-02-18T13:34:28Z

cmd/up/up.go

+	if !autoDeploy && !up.Dev.Autocreate {
+		err = errors.UserError{
+			E:    fmt.Errorf("Can't autocreate deployment"),
+			Hint: "Deploy your app first on okteto cloud or enable it using --deploy arg or setting autocreate variable to true in your manifest"}


@rlamana ^^ that's the error if running okteto up and the deployment doesn't exist

Here a few suggestions on how we should present this output:

Can we be more concise with the error? I wouldn't based it on the "autocreate" or at least I would make it clear that the deployment wasn't found first, and then maybe mention the possibility to add the autocreate in the hint. Maybe something like: Deployment ${name} not found in namespace '$namespace'

Don't mention Okteto Cloud since this could be any cluster, right?

In the hint show a clear list of options:

- Make sure your application is deployed and ready. - Make sure your context is pointing to the right namespace. - Set the `autocreate` property in your okteto.yml manifest if you want to automatically create a new development container.

Maybe we could show a link to the docs (are we doing this in other parts of the cli?).

wdyt?

pchico83 · 2021-02-18T13:37:04Z

cmd/up/up.go

-		if err := utils.AskIfDeploy(up.Dev.Name, up.Dev.Namespace); err != nil {
-			return nil, false, err
-		}
+	if !autoDeploy && !up.Dev.Autocreate {


Set dev.Autocreate to true here:
https://github.com/okteto/okteto/blob/master/cmd/up/up.go#L180
and from there, we only care about dev.Autocreate.

How will this interact with the CLI param? Will the CLI take precedence?

rberrelleza

See comments inline on the name of the key in the manifest.

rberrelleza · 2021-02-18T16:52:42Z

pkg/model/dev.go

@@ -144,6 +144,7 @@ type Dev struct {
 	Resources            ResourceRequirements  `json:"resources,omitempty" yaml:"resources,omitempty"`
 	Services             []*Dev                `json:"services,omitempty" yaml:"services,omitempty"`
 	PersistentVolumeInfo *PersistentVolumeInfo `json:"persistentVolume,omitempty" yaml:"persistentVolume,omitempty"`
+	Autocreate           bool                  `json:"autocreate,omitempty" yaml:"autocreate,omitempty"`


Is this the same as if the user passes the deploy arg in the CLI? If so, can we use the same name in the manifest to keep it consistent?

deploy sounds confusing? what about autodeploy?
Also, we are keeping the deploy arg, but it should be deprecated

I think calling the same thing two different ways is more confusing. People use the --deploy param today, so renaming to autodeploy it'll be confusing to them.

Both terms are equally confusing to me TBH (from the perspective of a new user), the fact that we call it 'deploy' or 'autocreate' is leaking that fact that we need a deployment before doing the up phase of the process. I'd rather stay with the current 'confusing' term rather than introduce yet another until we have a term we really like.

@rberrelleza do you have numbers on how many are using --deploy? The idea is to deprecate the deploy arg

not sure if we are collecting that information, but people that ask questions in the slack channel tend to use it a lot.

maybe we can call it mode: standalone? (with the idea that then having mode:swap in the future so that users can be more explicit about how they want to use up?)

if we are deprecating the deploy flag, maybe let's add a message to the help now?

I thought that the idea of using a mode was interesting for a long time. We even discussed this several times in the past, but the truth is that if we want to keep it simple that just adds a lot more complexity for a use case that is at its minimum:

According to the data:

95.59% of swap use.

4.41% of the "standalone" use. Of which we already know many happened by mistake.

Having a simple flag called autocreate: true right now (or even standalone: true) allows us to keep this feature for that small percentage who use it and avoids the current mistake of creating an unwanted deployment for some. I don't think we should be blocked by the naming convention for this, as long as is not as confusing as deploy or autodeploy, which is a term we use for something completely different.

I vote for autocreate: true or standalone: true

@jLopezbarb let's go with autocreate then

rberrelleza · 2021-02-18T16:53:31Z

cmd/up/up.go

-		if err := utils.AskIfDeploy(up.Dev.Name, up.Dev.Namespace); err != nil {
-			return nil, false, err
-		}
+	if !autoDeploy && !up.Dev.Autocreate {


How will this interact with the CLI param? Will the CLI take precedence?

Signed-off-by: jLopezbarb <javier@okteto.com>

codecov · 2021-02-18T19:09:38Z

Codecov Report

Merging #1289 (72bbe3e) into master (5ab5d6f) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
- Coverage   34.01%   33.94%   -0.08%     
==========================================
  Files          68       68              
  Lines        5780     5792      +12     
==========================================
  Hits         1966     1966              
- Misses       3607     3619      +12     
  Partials      207      207

Impacted Files	Coverage Δ
cmd/init/init.go	`11.95% <0.00%> (-0.07%)`	⬇️
cmd/up/up.go	`3.16% <0.00%> (-0.06%)`	⬇️
pkg/model/dev.go	`53.53% <ø> (ø)`

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 5ab5d6f...72bbe3e. Read the comment docs.

pchico83 · 2021-02-19T06:50:06Z

cmd/up/up.go

@@ -109,7 +109,7 @@ func Up() *cobra.Command {
 				return err
 			}

-			if err := loadDevOverrides(dev, namespace, k8sContext, forcePull, remote); err != nil {
+			if err := loadDevOverrides(dev, namespace, k8sContext, forcePull, remote, autoDeploy); err != nil {


There is an option (MarkHidden) in cobra to not show an arg in the help command. Let add the hidden option to the autodeploy flag and also remove autodeploy from the docs, but keeping the flag hidden for now

pchico83 · 2021-02-19T06:51:53Z

cmd/up/up.go

+		err = errors.UserError{
+			E: fmt.Errorf("Deployment %s not found in namespace '%s'", up.Dev.Name, up.Dev.Namespace),
+			Hint: `Please:
+	- Make sure your application is deployed and ready.


Suggested change

- Make sure your application is deployed and ready.

- Make sure your application is deployed.

pchico83 · 2021-02-19T06:52:04Z

cmd/up/up.go

+			E: fmt.Errorf("Deployment %s not found in namespace '%s'", up.Dev.Name, up.Dev.Namespace),
+			Hint: `Please:
+	- Make sure your application is deployed and ready.
+	- Make sure your context is pointing to the right namespace.


Suggested change

- Make sure your context is pointing to the right namespace.

- Make sure your context is pointing to the right namespace.

pchico83 · 2021-02-19T06:53:04Z

cmd/up/up.go

+			Hint: `Please:
+	- Make sure your application is deployed and ready.
+	- Make sure your context is pointing to the right namespace.
+	- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.


Suggested change

- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.

- Set the 'autocreate' field in your okteto manifest if you want to automatically create a new deployment in your current namespace.

What about a shorten version:

Suggested change

- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.

- To automatically create a new deployment, set the 'autocreate' field in your okteto manifest.

pchico83 · 2021-02-19T06:54:00Z

cmd/up/up.go

+	- Make sure your application is deployed and ready.
+	- Make sure your context is pointing to the right namespace.
+	- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.
+    More information is available here: https://okteto.com/docs/reference/cli#up`,


I think this link should point to the autocreate field docs, and add it to the third bullet point.

@pchico83 I talked to @jLopezbarb about this. I'd like to start adding links to the full documentation of the commands in case of error. I suggested this so users can read about the general workings of the UP command (including a link to the autocreate but from the docs themselves), instead of having many links on the same output.

We should then also add a paragraph explaining the 'autocreate' in general in the UP description and add a link to how to configure it in the manifest from there.

rlamana · 2021-02-19T09:47:52Z

@jLopezbarb can you share a screenshot of how the output looks in the terminal?

jLopezbarb · 2021-02-19T10:15:01Z

@rlamana this is how the output looks like with the url in the last bullet point.

I think something like adding another line to show the url could be more readable for the user.

rlamana · 2021-02-19T10:28:55Z

@rlamana this is how the output looks like with the url in the last bullet point.

I thought it was going to look worse :-P We should definitely shorten the last bullet point and take the URL to the next line.

rlamana · 2021-02-19T10:32:16Z

@rlamana this is how the output looks like with the url in the last bullet point.

We should also only point to the UP docs. It will make all that shorter too.

Signed-off-by: jLopezbarb <javier@okteto.com>

jLopezbarb · 2021-02-19T11:53:26Z

The warning message looks like this:

pchico83 · 2021-02-19T12:06:42Z

The warning message looks like this:

I would remove the "warning" word, it is already implied by the yellow color. @rlamana thoughts?

rberrelleza · 2021-02-19T18:24:06Z

cmd/up/up.go

@@ -104,12 +104,18 @@ func Up() *cobra.Command {

 			checkLocalWatchesConfiguration()

+			if autoDeploy {
+				log.Yellow(`Warning: The use of 'deploy' flag will be deprecated in a future release. 
+    	 Please set 'autocreate' in your okteto manifest to get the same behaviour. 


Suggested change

Please set 'autocreate' in your okteto manifest to get the same behaviour.

Please set 'autocreate' in your okteto manifest to get the same behavior.

rlamana · 2021-02-22T10:33:51Z

I would remove the "warning" word, it is already implied by the yellow color. @rlamana thoughts?
@pchico83 @jLopezbarb I would remove it too. But, don't we have a symbol for this type of message, just like we have the [x] for errors, or the [i] for info messages? I think we should use that format too (maybe we can have an [!] exclamation symbol.

pchico83 · 2021-02-22T10:36:43Z

I would remove the "warning" word, it is already implied by the yellow color. @rlamana thoughts?
@pchico83 @jLopezbarb I would remove it too. But, don't we have a symbol for this type of message, just like we have the [x] for errors, or the [i] for info messages? I think we should use that format too (maybe we can have an [!] exclamation symbol.

@rlamana we don't but makes sense. Let's create an issue for it to keep the scope of this issue unaffected.

Signed-off-by: jLopezbarb <javier@okteto.com>

derek · 2021-02-23T09:19:30Z

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.

Add autocreate field to manifest

cba730b

Signed-off-by: jLopezbarb <javier@okteto.com>

jLopezbarb requested review from pchico83, rberrelleza and rlamana as code owners February 18, 2021 13:31

pchico83 reviewed Feb 18, 2021

View reviewed changes

rberrelleza reviewed Feb 18, 2021

View reviewed changes

jLopezbarb added 2 commits February 18, 2021 17:59

Set autodeploy and autocreate to the same variable

7b27052

Signed-off-by: jLopezbarb <javier@okteto.com>

Update hint message

1d8fac7

Signed-off-by: jLopezbarb <javier@okteto.com>

pchico83 reviewed Feb 19, 2021

View reviewed changes

jLopezbarb added 2 commits February 19, 2021 12:48

Update hint message

97cbba6

Signed-off-by: jLopezbarb <javier@okteto.com>

Add deploy deprecation warning

018bf21

Signed-off-by: jLopezbarb <javier@okteto.com>

rberrelleza reviewed Feb 19, 2021

View reviewed changes

jLopezbarb and others added 3 commits February 22, 2021 12:32

Update warning message

0cc6f08

Signed-off-by: jLopezbarb <javier@okteto.com>

Add "autocreate" on "okteto init" if needed

5ccc9f6

Update info messages

72bbe3e

derek bot added the no-dco label Feb 23, 2021

pchico83 merged commit 563eafa into master Feb 23, 2021

pchico83 deleted the jlopezbarb/autocreate-manifest-field branch February 23, 2021 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add autocreate field to manifest #1289

Add autocreate field to manifest #1289

jLopezbarb commented Feb 18, 2021

pchico83 Feb 18, 2021

rlamana Feb 18, 2021

pchico83 Feb 18, 2021

rberrelleza Feb 18, 2021

rberrelleza left a comment

rberrelleza Feb 18, 2021

pchico83 Feb 18, 2021

rberrelleza Feb 18, 2021 •

edited

pchico83 Feb 18, 2021

rberrelleza Feb 18, 2021 •

edited

rberrelleza Feb 18, 2021

rlamana Feb 19, 2021 •

edited

pchico83 Feb 19, 2021

rberrelleza Feb 18, 2021

codecov bot commented Feb 18, 2021 •

edited

pchico83 Feb 19, 2021

pchico83 Feb 19, 2021

pchico83 Feb 19, 2021

pchico83 Feb 19, 2021

rlamana Feb 19, 2021

pchico83 Feb 19, 2021

rlamana Feb 19, 2021

rlamana commented Feb 19, 2021

jLopezbarb commented Feb 19, 2021 •

edited

rlamana commented Feb 19, 2021

rlamana commented Feb 19, 2021

jLopezbarb commented Feb 19, 2021

pchico83 commented Feb 19, 2021

rberrelleza Feb 19, 2021

rlamana commented Feb 22, 2021

pchico83 commented Feb 22, 2021

derek bot commented Feb 23, 2021

	- Make sure your application is deployed and ready.
	- Make sure your application is deployed.

	- Make sure your context is pointing to the right namespace.
	- Make sure your context is pointing to the right namespace.

	- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.
	- Set the 'autocreate' field in your okteto manifest if you want to automatically create a new deployment in your current namespace.

	- Set the 'autocreate' property in your okteto.yml manifest if you want to automatically create a new development container.
	- To automatically create a new deployment, set the 'autocreate' field in your okteto manifest.

	Please set 'autocreate' in your okteto manifest to get the same behaviour.
	Please set 'autocreate' in your okteto manifest to get the same behavior.

Add autocreate field to manifest #1289

Add autocreate field to manifest #1289

Conversation

jLopezbarb commented Feb 18, 2021

Proposed changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rberrelleza Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rberrelleza Feb 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlamana Feb 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2021 • edited

Codecov Report

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

rlamana commented Feb 19, 2021

jLopezbarb commented Feb 19, 2021 • edited

rlamana commented Feb 19, 2021

rlamana commented Feb 19, 2021

jLopezbarb commented Feb 19, 2021

pchico83 commented Feb 19, 2021

Choose a reason for hiding this comment

rlamana commented Feb 22, 2021

pchico83 commented Feb 22, 2021

derek bot commented Feb 23, 2021

rberrelleza Feb 18, 2021 •

edited

rberrelleza Feb 18, 2021 •

edited

rlamana Feb 19, 2021 •

edited

codecov bot commented Feb 18, 2021 •

edited

jLopezbarb commented Feb 19, 2021 •

edited