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

feat: Implement solo context connect #863

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Ivo-Yankov
Copy link
Contributor

@Ivo-Yankov Ivo-Yankov commented Nov 22, 2024

  • Added a new solo context connect command.
  • Added new flags for context and clusterName
  • Added validation error messages and updated error handling for local configuration.

solo context connect accepts the following flags:

  • --namespace
  • --context
  • --cluster-name
  • --quiet-mode

When a flag is not specified the user will be prompted to input a value. For context a list of all contexts from kubectl config is provided. When quiet=true the default values from kubectl are used.

Currently the command utilizes the promptLocalConfigTask task, which creates the local-config.yaml file if it doesn't exist and prompts for userEmailAddress and for deployments. In a future PR this will be changed so that a different command will be responsible for instantiating the local configuration.

Related Issues

Signed-off-by: Ivo Yankov <[email protected]>
Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Test Results - Linux

  1 files  ±0   38 suites  +2   3s ⏱️ ±0s
116 tests +7  116 ✅ +7  0 💤 ±0  0 ❌ ±0 
125 runs  +7  125 ✅ +7  0 💤 ±0  0 ❌ ±0 

Results for commit d48164c. ± Comparison against base commit 308f39d.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Test Results - Windows

  1 files  ±0   38 suites  +2   14s ⏱️ -1s
116 tests +7  116 ✅ +7  0 💤 ±0  0 ❌ ±0 
125 runs  +7  125 ✅ +7  0 💤 ±0  0 ❌ ±0 

Results for commit d48164c. ± Comparison against base commit 308f39d.

♻️ This comment has been updated with latest results.

…of-2-updates-the-localconfig-by-connecting-a-deployment-to-a-k8s-context

# Conflicts:
#	src/commands/node/handlers.ts
#	src/commands/node/tasks.ts
#	src/core/k8.ts
Signed-off-by: Ivo Yankov <[email protected]>
Copy link
Contributor

github-actions bot commented Nov 26, 2024

E2E Test Report

 13 files  ±0   94 suites  ±0   55m 40s ⏱️ +11s
200 tests +1  200 ✅ +1  0 💤 ±0  0 ❌ ±0 
203 runs  +1  203 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit d48164c. ± Comparison against base commit 308f39d.

♻️ This comment has been updated with latest results.

Copy link

codacy-production bot commented Nov 26, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.15% (target: -1.00%) 69.29%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (308f39d) 14906 12666 84.97%
Head commit (d48164c) 15239 (+333) 12926 (+260) 84.82% (-0.15%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#863) 674 467 69.29%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Nov 26, 2024

Codecov Report

Attention: Patch coverage is 68.54599% with 212 lines in your changes missing coverage. Please review.

Project coverage is 83.62%. Comparing base (308f39d) to head (d48164c).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/commands/prompts.ts 55.50% 105 Missing ⚠️
src/commands/context/handlers.ts 46.93% 26 Missing ⚠️
src/commands/context/index.ts 51.85% 26 Missing ⚠️
src/commands/context/tasks.ts 85.14% 13 Missing and 2 partials ⚠️
src/core/config/local_config.ts 61.53% 14 Missing and 1 partial ⚠️
src/core/templates.ts 41.17% 10 Missing ⚠️
src/commands/node/tasks.ts 43.75% 9 Missing ⚠️
src/core/k8.ts 92.45% 4 Missing ⚠️
src/commands/index.ts 33.33% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
- Coverage   83.81%   83.62%   -0.19%     
==========================================
  Files          49       54       +5     
  Lines       14934    15239     +305     
  Branches     1034     1126      +92     
==========================================
+ Hits        12517    12744     +227     
- Misses       2401     2436      +35     
- Partials       16       59      +43     
Files with missing lines Coverage Δ
src/commands/base.ts 95.88% <100.00%> (+0.20%) ⬆️
src/commands/context/flags.ts 100.00% <100.00%> (ø)
src/commands/flags.ts 99.57% <100.00%> (+<0.01%) ⬆️
src/commands/node/handlers.ts 81.24% <100.00%> (+0.02%) ⬆️
src/core/error_messages.ts 100.00% <100.00%> (ø)
src/core/helpers.ts 80.84% <100.00%> (+0.25%) ⬆️
src/core/task.ts 100.00% <100.00%> (ø)
src/core/validator_decorators.ts 98.00% <ø> (-0.04%) ⬇️
src/commands/index.ts 62.50% <33.33%> (-1.66%) ⬇️
src/core/k8.ts 89.91% <92.45%> (+0.08%) ⬆️
... and 7 more

... and 11 files with indirect coverage changes

Impacted file tree graph

…of-2-updates-the-localconfig-by-connecting-a-deployment-to-a-k8s-context
@Ivo-Yankov Ivo-Yankov marked this pull request as ready for review November 26, 2024 11:03
@Ivo-Yankov Ivo-Yankov requested review from leninmehedy and a team as code owners November 26, 2024 11:03
@Ivo-Yankov Ivo-Yankov changed the title feat: Implement solo context use feat: Implement solo context connect Nov 26, 2024

export const DEFAULT_FLAGS = {
requiredFlags: [],
requiredFlagsWithDisabledPrompt: [flags.namespace, flags.cacheDir, flags.releaseTag],
Copy link
Contributor

@jeromy-cannon jeromy-cannon Nov 27, 2024

Choose a reason for hiding this comment

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

what makes releaseTag required or used for solo context connect?

Comment on lines 82 to 85
this.parent.logger.info(`currentDeploymentName: ${currentDeploymentName}`)
this.parent.logger.info(`contextName: ${contextName}`)
this.parent.logger.info(`clusterAliases: ${clusterAliases.join(' ')}`)
this.parent.logger.info('Save LocalConfig file')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.parent.logger.info(`currentDeploymentName: ${currentDeploymentName}`)
this.parent.logger.info(`contextName: ${contextName}`)
this.parent.logger.info(`clusterAliases: ${clusterAliases.join(' ')}`)
this.parent.logger.info('Save LocalConfig file')
this.parent.logger.info('Save LocalConfig file: [currentDeploymentName: ${currentDeploymentName}, contextName: ${contextName}, clusterAliases: ${clusterAliases.join(' ')}]')

less verbose in the log file if it is on one line

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall us talking about storing the cluster to context mapping by reading it in, but not saving it to the local config file, so that it would be rebuilt each time and can be used by other parts of the application. Maybe I'll find it somewhere else while reviewing this PR?

src/core/task.ts Outdated
Comment on lines 18 to 22
constructor (public title: string, public task: Function, public skip: Function | boolean = false) {
this.title = title
this.task = task
this.skip = skip
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constructor (public title: string, public task: Function, public skip: Function | boolean = false) {
this.title = title
this.task = task
this.skip = skip
}
constructor (public title: string, public task: Function, public skip: Function | boolean = false) { }

I think this means the same thing?

src/core/k8.ts Outdated
.then(conn => {
self.logger.info(`${messagePrefix} connection established`)
localContext.connection = conn
({ status }) => self.handleCallback(status, localContext, messagePrefix))
Copy link
Contributor

Choose a reason for hiding this comment

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

what formatter are you using? I noticed that npm run format does not do this. I have an outstanding item to implement prettier. although now that we agreed on google style, there is probably a better way.

In IntelliJ, I had the Palantir Style set, previously in Java, so some of those settings carried over. If I reformat this file, it changes everything back to how I had it. We need to all get on the same page and use the same settings.

constName: 'contextName',
name: 'context',
definition: {
describe: 'The kind context name to be used',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe: 'The kind context name to be used',
describe: 'The Kubernetes context name to be used',

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants