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

set okteto context and namespace from env vars #4247

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

andreafalzetti
Copy link
Contributor

@andreafalzetti andreafalzetti commented Apr 17, 2024

Proposed changes

Fixes DEV-281

How to validate

You need at least 2 different contexts to validate this change.

Scenario 1 - no context

Without any current context, or --context, or OKTETO_CONTEXT the CLI should tell you that you have no context.

Try the following:

OKTETO_HOME=$(mktemp -d) okteto endpoints
# or
OKTETO_HOME=$(mktemp -d) okteto context show

And validate that you get a message in both cases saying your context is not set.

Scenario 2 - current context

Assuming you have a current context:

  1. run okteto context show
  2. validate that the context is correct

Scenario 3 - current context + --context flag

The flag should have priority over the current context, to validate this scenario, run:

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. Run okteto endpoints --context <CTX2>
  4. the CLI should say you are using <CTX2>
  5. check that the namespace is also selected correctly based on <CTX2>
  6. now run: okteto endpoints --context <CTX2> --namespace <NS2>
  7. check that the <NS2> is picked up as expected

Scenario 4 - current context + OKTETO_CONTEXT

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. Run OKTETO_CONTEXT=<CTX2> okteto endpoints
  4. the CLI should say you are using <CTX2>
  5. check that the namespace is also selected correctly based on <CTX2>
  6. now run: OKTETO_CONTEXT=<CTX2> OKTETO_NAMESPACE=<NS2> okteto endpoints
  7. check that the <NS2> is picked up as expected

Scenario 5 - current context + OKTETO_CONTEXT + --context

For this scenario it's ideal to use 3 different contextes to avoid ambiguities.

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. Run OKTETO_CONTEXT=<CTX2> okteto endpoints --context <CTX3>
  4. the CLI should say you are using <CTX3>
  5. check that the namespace is also selected correctly based on <CTX3>
  6. now run: OKTETO_CONTEXT=<CTX2> OKTETO_NAMESPACE=<NS2> okteto endpoints --context <CTX3> --namespace <NS3>
  7. check that the <NS3> is picked up as expected

Scenario 6 - current context + .env with OKTETO_CONTEXT + --context

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. create .env with OKTETO_CONTEXT=<CTX2>
  4. Run okteto endpoints --context <CTX3>
  5. the CLI should say you are using <CTX3>
  6. check that the namespace is also selected correctly based on <CTX3>
  7. now add OKTETO_NAMESPACE=<NS2> to the .env
  8. Run: okteto endpoints --context <CTX3> --namespace <NS3>
  9. check that the <NS3> is picked up as expected

Scenario 7 - current context + manifest context: + --context

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. set context: <CTX2> in your manifest
  4. Run okteto endpoints --context <CTX3>
  5. the CLI should say you are using <CTX2>
  6. check that the namespace is also selected correctly based on <CTX2>
  7. now add namespace: <NS2> in your manifest
  8. now run: okteto endpoints --context <CTX3> --namespace <NS3>
  9. check that the <NS2> is picked up as expected

Scenario 8 - current context + OKTETO_URL

  1. run okteto context show
  2. observe which context you have as current (let's callit <CTX1>)
  3. Run OKTETO_URL=<CTX2> okteto endpoints
  4. the CLI should say you are using <CTX2>
  5. check that the namespace is also selected correctly based on <CTX2>
  6. now run: OKTETO_URL=<CTX2> OKTETO_NAMESPACE=<NS2> okteto endpoints
  7. check that the <NS2> is picked up as expected

Out of scope scenarios:

These are lower-priority scenarios that may be worked on with a follow-up ticket/PR.

  • Scenario 8 - current context + OKTETO_CONTEXT var set in Catalog
  • Scenario 9 - current context + OKTETO_CONTEXT var set in Catalog + --context
  • Scenario 10 - current context + OKTETO_CONTEXT set as --var flag in deploy
  • Scenario 11 - current context + OKTETO_CONTEXT set as User variable
  • Scenario 12- current context + OKTETO_CONTEXT set as Cluster variable

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

@andreafalzetti andreafalzetti force-pushed the af/dev-281-okteto-context-and-namespace-env-vars branch from caf5e56 to 668bee0 Compare April 19, 2024 14:28
@andreafalzetti andreafalzetti marked this pull request as ready for review April 22, 2024 09:10
@andreafalzetti andreafalzetti requested a review from a team as a code owner April 22, 2024 09:10
Copy link
Contributor

@maroshii maroshii left a comment

Choose a reason for hiding this comment

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

I don't have enough context in the CLI or understand how context works internally to confidently approve this PR. Deferring approval to @jLopezbarb

@andreafalzetti andreafalzetti force-pushed the af/dev-281-okteto-context-and-namespace-env-vars branch from c7f0a8f to 8cc6430 Compare April 24, 2024 12:09
@andreafalzetti andreafalzetti added the run-e2e When used on a PR run windows & unix e2e label Apr 24, 2024
@andreafalzetti andreafalzetti marked this pull request as draft April 26, 2024 10:52
@andreafalzetti
Copy link
Contributor Author

Moving to draft until e2e are passing

@andreafalzetti andreafalzetti force-pushed the af/dev-281-okteto-context-and-namespace-env-vars branch 3 times, most recently from 23e504f to 0087d3d Compare April 30, 2024 12:27
@andreafalzetti andreafalzetti marked this pull request as ready for review April 30, 2024 13:21
Copy link
Contributor

@jLopezbarb jLopezbarb left a comment

Choose a reason for hiding this comment

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

LGTM besides of the e2e tests

@andreafalzetti andreafalzetti force-pushed the af/dev-281-okteto-context-and-namespace-env-vars branch from 0087d3d to 9a6c151 Compare April 30, 2024 18:00
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
Signed-off-by: Andrea Falzetti <andrea@okteto.com>
@andreafalzetti andreafalzetti force-pushed the af/dev-281-okteto-context-and-namespace-env-vars branch from 9a6c151 to e8ea18e Compare May 2, 2024 16:31
@andreafalzetti andreafalzetti marked this pull request as draft May 3, 2024 08:54
@andreafalzetti andreafalzetti marked this pull request as ready for review May 7, 2024 17:19
@andreafalzetti andreafalzetti added the backport release-2.27 Backport this PR to CLI version 2.27 label May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 6.25000% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 45.64%. Comparing base (06d3161) to head (59963f7).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4247      +/-   ##
==========================================
- Coverage   45.64%   45.64%   -0.01%     
==========================================
  Files         308      308              
  Lines       27849    27839      -10     
==========================================
- Hits        12711    12706       -5     
+ Misses      14040    14035       -5     
  Partials     1098     1098              

@maroshii
Copy link
Contributor

maroshii commented May 7, 2024

This looks like quite a sensitive change for a backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-2.27 Backport this PR to CLI version 2.27 release/new-feature run-e2e When used on a PR run windows & unix e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants