Skip to content

Conversation

@thaJeztah
Copy link
Member

While we should produce an error when trying to use the context, it's ok to look up the currently configured context (name) without validating if it's valid (or exists). This helps with situations where (e.g.) an invalid context is configured, but the user only needs to run "local" commands that don't require a server connection.

Note that the above is not yet possible with this fix alone (will be addressed in #3847)

cli/command: resolveContextName() don't validate if context exists

resolveContextName() is used to find which context to use, based on the
available configuration options. Once resolved, the context name is
used to load the actual context, which will fail if the context doesn't
exist, so there's no need to produce an error at this stage; only
check priority of the configuration options to pick the context
with the highest priority.

cli/command: resolveContextName() move conflicting options check

There's no strict need to perform this validation inside this function;
validating flags should happen earlier, to allow faster detecting of
configuration issues (we may want to have a central config "validate"
function though).

cli/command: DockerCli.CurrentContext: improve GoDoc

Also move the resolveContextName() function together with the
method for easier cross-referencing.

resolveContextName() is used to find which context to use, based on the
available configuration options. Once resolved, the context name is
used to load the actual context, which will fail if the context doesn't
exist, so there's no need to produce an error at this stage; only
check priority of the configuration options to pick the context
with the highest priority.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There's no strict need to perform this validation inside this function;
validating flags should happen earlier, to allow faster detecting of
configuration issues (we may want to have a central config "validate"
function though).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added kind/enhancement area/context kind/refactor PR's that refactor, or clean-up code labels Nov 22, 2022
@thaJeztah thaJeztah added this to the 22.06.0 milestone Nov 22, 2022
@thaJeztah
Copy link
Member Author

Something failing; perhaps I need some bits from the other commits. Let me check

#19 85.35 === FAIL: cli/command TestInitializeFromClientHangs (1.00s)
#19 85.35     cli_test.go:215: server never received an init request

Also move the resolveContextName() function together with the
method for easier cross-referencing.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the current_context_improvements branch from 3a02f66 to e36d5a0 Compare November 22, 2022 13:28
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

Merging #3878 (323c869) into master (de52868) will increase coverage by 0.01%.
The diff coverage is 38.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3878      +/-   ##
==========================================
+ Coverage   59.22%   59.23%   +0.01%     
==========================================
  Files         287      287              
  Lines       24662    24656       -6     
==========================================
- Hits        14606    14605       -1     
+ Misses       9172     9168       -4     
+ Partials      884      883       -1     

@thaJeztah thaJeztah force-pushed the current_context_improvements branch 2 times, most recently from 2dd9d0c to 323c869 Compare November 22, 2022 15:23
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the current_context_improvements branch from 323c869 to 9c42cd9 Compare November 22, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/context kind/enhancement kind/refactor PR's that refactor, or clean-up code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants