Fix `okteto endpoints` namespace detection from `okteto.yml` #4186

andreafalzetti · 2024-02-20T09:59:22Z

Proposed changes

okteto endpoints ignores the property namespace of the okteto.yml.

How to validate

Create a new namespace where you have nothing deployed to (from now referred as NS1)
Select NS1 as your current default
Using any project (ie. https://github.com/okteto/movies-frontend)
Modify the okteto.yml and add a top-level namespace: <NS2> (replace <NS2> with a namespace different from NS1)
Run okteto endpoints and observe how the CLI outputs Using <NS1> @ <Context> as context
It's worth noticing that the endpoints are printed correctly, it's just the namespace name that is wrong (The CLI uses the correct one, but it displayes the wrong one)

Build the CLI from this PR and repeat the steps:

Observe how okteto endpoints print the correct namespace
Deploy the app and observe the right endpoints printer for each NS

Extra:
Currently if you run okteto endpoints -f not-exists.yml the endoints cmd falls back on okteto.yml. This behavior is inconsistent with other cmds so I've changed it to return an error (like okteto deploy).

Test Scenarios

okteto endpoints
okteto endpoints -n <NS>
okteto endpoints with namespace: <NS>

CLI Quality Reminders 🔧

For both authors and reviewers:

Scrutinize for potential regressions
Ensure key automated tests are in place
Build the CLI and test using the validation steps
Assess Developer Experience impact (log messages, performances, etc)
If too broad, consider breaking into smaller PRs
Adhere to our code style and code review guidelines

… file Signed-off-by: Andrea Falzetti <andrea@okteto.com>

andreafalzetti · 2024-02-20T10:05:34Z

cmd/deploy/endpoints.go

-			}
-			if err := contextCMD.NewContextCommand().Run(ctx, ctxOptions); err != nil {
-				return err
+			// Loads, updates and uses the context from path. If not found, it creates and uses a new context


This is the same logic used in okteto deploy

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

codecov · 2024-02-20T11:35:21Z

Codecov Report

Merging #4186 (231827a) into master (31cbb7c) will increase coverage by 0.03%.
Report is 3 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4186      +/-   ##
==========================================
+ Coverage   45.55%   45.59%   +0.03%     
==========================================
  Files         289      289              
  Lines       27108    27101       -7     
==========================================
+ Hits        12350    12356       +6     
+ Misses      13672    13661      -11     
+ Partials     1086     1084       -2

ifbyol

Testing it I see that with the code in this PR, the context used is not printed in the CLI output.

You can see that when I use ok (the binary built with the changes in this branch), the context is not displayed and it directly shows the endpoints

If I use okteto (latest stable CLI version), it shows the context used (the wrong one)

ifbyol · 2024-02-21T08:12:05Z

cmd/context/utils.go

@@ -224,7 +224,7 @@ func LoadStackWithContext(ctx context.Context, name, namespace string, stackPath
 }

 // LoadContextFromPath initializes the okteto context taking into account command flags and manifest namespace/context fields
-func LoadContextFromPath(ctx context.Context, namespace, k8sContext, path string) error {
+func LoadContextFromPath(ctx context.Context, namespace, k8sContext, path string, defaultCtxOpts *Options) error {


Why do we need a pointer for defaultCtxOpts? I see we always pass a value. We could just pass the value

no need to be pointer actually, the code is much better with a value type, changed in e0b7d6b

thanks!

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

andreafalzetti · 2024-02-21T10:37:32Z

Testing it I see that with the code in this PR, the context used is not printed in the CLI output.

You can see that when I use ok (the binary built with the changes in this branch), the context is not displayed and it directly shows the endpoints

If I use okteto (latest stable CLI version), it shows the context used (the wrong one)

Good catch @ifbyol! I made the Show dynamic and forgot to revert a change I had to make it show depending on the output flag. 🙇

ifbyol · 2024-02-21T20:25:17Z

cmd/deploy/endpoints.go

-			if err := contextCMD.NewContextCommand().Run(ctx, ctxOptions); err != nil {
-				return err
+			// Loads, updates and uses the context from path. If not found, it creates and uses a new context
+			if err := contextCMD.LoadContextFromPath(ctx, options.Namespace, options.K8sContext, options.ManifestPath, contextCMD.Options{Show: options.Output == ""}); err != nil {


Why for the endpoints command the Show value would depend on the options.Output value but for the deploy and destroy command it is always true. What is special about the endpoints command?

the okteto endpoints command can be used as follows:

okteto endpoints --output=json | jq XYZ

or

okteto endpoints --output=md > README.md

In those scenarios we cannot output the plaintext header or it would break those scenarios. For build and deploy we don't have such use-cases, or at least I am not aware or them, so the Show value - only for endpoints depend on its --output flag.

Worth mentioning that for "plaintext" in most commands of the CLI we use empty string. So by default an empty --output flag means "plaintext". So the check is a bit odd. I hope that with 3.0 we can add a third allowed value for --output plaintext or --output text to make this more explicit.

For deploy and destroy we have the global flag --log-output which could be also json (I was confusing the flag output with that global flag). I was testing deploy and destroy, and when the --log-output flag is set, we are not displaying the context information even if we set Show: true in the context.

I see it working fine but I think this is confusing for the user. If I execute okteto endpoints --log-output=json it doesn't print anything in the output

ifbyol · 2024-02-22T08:30:03Z

I was testing it and If I specify the namespace through the flag -n, we don't show the context information, but prior this change, we were displaying it

andreafalzetti · 2024-02-22T11:12:34Z

I was testing it and If I specify the namespace through the flag -n, we don't show the context information, but prior this change, we were displaying it

Good catch, I think the scenario you reproduced is because when you specify the namespace in both places (manifest + flag), contextCMD.LoadContextFromPath fails (due to error oktetoErrors.ErrNamespaceNotMatching from the func UpdateNamespace), calling the fallback logic contextCMD.NewContextCommand().Run, which had Show: false hardcoded.

After looking at why it had Show: false hardcoded, it seems to me a safeguard to avoid showing the header twice. In fact, in case of error occurring after the display of the header in the first func, the second func might display it again. Considering that the display is done just before a return nil, I don't think it's risky to pass a dynamic Show in both functions.

However, keeping false will never make the header appear, so it's a conflicting logic, so I've pushed a new commit with this change, fixing this

ifbyol · 2024-02-22T12:01:13Z

cmd/deploy/endpoints.go

-			if err := contextCMD.NewContextCommand().Run(ctx, ctxOptions); err != nil {
-				return err
+			// false for 'json' and 'md' to avoid breaking their syntax
+			showCtxHeader := options.Output == ""


👏 Thanks for the comment clarifying the case

andreafalzetti added the release/bug-fix label Feb 20, 2024

derek bot added the no-dco label Feb 20, 2024

This comment was marked as outdated.

Sign in to view

andreafalzetti added run-e2e-unix When used on a PR run unix e2e and removed no-dco labels Feb 20, 2024

This comment was marked as outdated.

Sign in to view

derek bot added the no-dco label Feb 20, 2024

andreafalzetti force-pushed the fix/okteto-endpoints branch from 5ded92b to 4994853 Compare February 20, 2024 10:01

derek bot removed the no-dco label Feb 20, 2024

fix(endpoints): detect namespace from manifest and check for manifest…

a4e7a04

… file Signed-off-by: Andrea Falzetti <andrea@okteto.com>

andreafalzetti force-pushed the fix/okteto-endpoints branch from 4994853 to a4e7a04 Compare February 20, 2024 10:02

andreafalzetti changed the title ~~Fix/okteto endpoints~~ Fix okteto endpoints namespace detection from okteto.yml Feb 20, 2024

andreafalzetti commented Feb 20, 2024

View reviewed changes

andreafalzetti added 4 commits February 20, 2024 11:07

update error msg

bcf3330

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

fix linter

3a313a3

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

fix: endpoints output

f771f39

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

revert unecessary change

f512832

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

ifbyol reviewed Feb 21, 2024

View reviewed changes

andreafalzetti added 2 commits February 21, 2024 11:27

fix: header

04d2425

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

change default options not pointer

e0b7d6b

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

andreafalzetti marked this pull request as ready for review February 21, 2024 10:38

andreafalzetti requested a review from a team as a code owner February 21, 2024 10:38

ifbyol reviewed Feb 21, 2024

View reviewed changes

fix: handle case when namespace is provided in both places

231827a

This comment was marked as off-topic.

Sign in to view

derek bot added the no-dco label Feb 22, 2024

ifbyol approved these changes Feb 22, 2024

View reviewed changes

andreafalzetti merged commit a1e8c96 into master Feb 23, 2024
16 of 18 checks passed

andreafalzetti deleted the fix/okteto-endpoints branch February 23, 2024 08:28

BrewTestBot mentioned this pull request Apr 16, 2024

okteto 2.26.0 Homebrew/homebrew-core#169189

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `okteto endpoints` namespace detection from `okteto.yml` #4186

Fix `okteto endpoints` namespace detection from `okteto.yml` #4186

andreafalzetti commented Feb 20, 2024 •

edited

This comment was marked as outdated.

This comment was marked as outdated.

andreafalzetti Feb 20, 2024

codecov bot commented Feb 20, 2024 •

edited

ifbyol left a comment

ifbyol Feb 21, 2024

andreafalzetti Feb 21, 2024

andreafalzetti commented Feb 21, 2024 •

edited

ifbyol Feb 21, 2024

andreafalzetti Feb 21, 2024

ifbyol Feb 22, 2024

ifbyol commented Feb 22, 2024

This comment was marked as off-topic.

andreafalzetti commented Feb 22, 2024

ifbyol Feb 22, 2024

Fix okteto endpoints namespace detection from okteto.yml #4186

Fix okteto endpoints namespace detection from okteto.yml #4186

Conversation

andreafalzetti commented Feb 20, 2024 • edited

Proposed changes

How to validate

Test Scenarios

CLI Quality Reminders 🔧

This comment was marked as outdated.

This comment was marked as outdated.

andreafalzetti Feb 20, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2024 • edited

Codecov Report

ifbyol left a comment

Choose a reason for hiding this comment

ifbyol Feb 21, 2024

Choose a reason for hiding this comment

andreafalzetti Feb 21, 2024

Choose a reason for hiding this comment

andreafalzetti commented Feb 21, 2024 • edited

ifbyol Feb 21, 2024

Choose a reason for hiding this comment

andreafalzetti Feb 21, 2024

Choose a reason for hiding this comment

ifbyol Feb 22, 2024

Choose a reason for hiding this comment

ifbyol commented Feb 22, 2024

This comment was marked as off-topic.

andreafalzetti commented Feb 22, 2024

ifbyol Feb 22, 2024

Choose a reason for hiding this comment

Fix `okteto endpoints` namespace detection from `okteto.yml` #4186

Fix `okteto endpoints` namespace detection from `okteto.yml` #4186

andreafalzetti commented Feb 20, 2024 •

edited

codecov bot commented Feb 20, 2024 •

edited

andreafalzetti commented Feb 21, 2024 •

edited