Add a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11657

KshitijThareja · 2023-12-16T12:43:34Z

Summary

This PR aims to make the yarn run devserver-with-kds command throw an error warning when KDS path is not provided while running it in the cli.

References

#11632

Reviewer guidance

Run the yarn run devserver-with-kds command in the cli without specifying the path to the local kds.
You'll see an error message and the command will terminate with exit code 1.

Screenshot

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

PR process

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

github-actions · 2023-12-16T13:17:13Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b10.dev0_git.35.g4814496b.pex
Windows Installer (EXE)	kolibri-0.16.0b10.dev0+git.35.g4814496b-unsigned.exe
Debian Package	kolibri_0.16.0b10.dev0+git.35.g4814496b-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b10.dev0+git.35.g4814496b-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b10.dev0+git.35.g4814496b-0.1.0-debug.apk
TAR file	kolibri-0.16.0b10.dev0+git.35.g4814496b.tar.gz
WHL file	kolibri-0.16.0b10.dev0+git.35.g4814496b-py2.py3-none-any.whl

KshitijThareja · 2023-12-16T13:17:31Z

@MisRob I had to use the --kds flag here (as also suggested by @Jaspreet-singh-1032) because without it, I don't think there's any direct way to know if the yarn command is run using devserver or devserver-with-kds.
If there are any suggestions from your side, I'd be happy to work on them.

nucleogenesis

I love the introduction of an error message here. This will make things much easier to work out why something has gone awry when it inevitably does. Thank you for your work on this!

Each of my review comments is merge-blocking. Please let me know if you have any questions!

nucleogenesis · 2023-12-18T22:21:49Z

package.json

@@ -26,7 +26,7 @@
    "app-devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:app-python-devserver yarn:hashi-dev --",
    "devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:python-devserver yarn:hashi-dev --",
    "devserver-hot": "concurrently --passthrough-arguments --kill-others \"yarn:watch-hot --watchonly {1}\" yarn:python-devserver yarn:hashi-dev --",
-    "devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --",
+    "devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly --kds --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --",


One thing that I think might be an issue here is that this seems like it will disallow the use of the baked-in --watchonly call which can be passed a comma-separated list of plugins like so IIRC:

yarn devserver learn,coach which will restrict the build process to those Kolibri plugins.

The --kds-path is swallowing the entirety of what is passes in as the --watchonly option will never receive any user input.

So we cannot call yarn devserver-with-kds learn,coach to only build those plugins because the resulting call will be

kolibri-tools build dev --file ./build_tools/build_plugins.txt --cache --watchonly --kds-path=learn,coach

Perhaps we can maintain the functionality by changing this line to:

"devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly={2} --kds --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --"

(note the ={2} added after --watchonly)

In this case, we can call: yarn devserver-with-kds ../path/to/kds learn,coach which would result in this call:

kolibri-tools build dev --file ./build_tools/build_plugins.txt --cache --watchonly=learn,coach --kds-path=../path/to/kds

And this should fix the issue here.

nucleogenesis · 2023-12-18T22:24:36Z

packages/kolibri-tools/lib/cli.js

@@ -226,8 +227,15 @@ program
    list,
    []
  )
+  .option('--kds', 'Flag to use local kds', false)


I think that maybe the description here isn't entirely true. You don't need this flag to run with local KDS but rather you just need this flag if you want to get errors related to your attempts to use local KDS.

It might be better to call this something like --require-kds-path to better indicate what it is really doing.

nucleogenesis · 2023-12-18T22:28:02Z

packages/kolibri-tools/lib/cli.js

  .option('--kds-path <kdsPath>', 'Full path to local kds directory', String, '')
  .action(function(mode, options) {
+    if (options.kds) {
+      if (!options.kdsPath) {
+        cliLogging.error('Path to the local KDS directory not specified.');


I think updating this to indicate why the error was received will help here. Like "The --require-kds-path flag was given but no --kds-path value was given. Please provide the local KDS directory" (or something like that).

KshitijThareja · 2023-12-19T16:02:06Z

Hi @nucleogenesis. Thanks for your suggestions. I've made all the required changes as requested. Can you please go through them once and check if there are any more changes needed?

nucleogenesis

Thank you @KshitijThareja !

I've tested the failed state and got the error as expected.

I also tested by updating KTextbox in my and the files were being watched and rebuilt when I saved the file then I saw my changes:

I'll be drafting a follow-up issue re: the --watchonly flag separately. I'm not super sure how best to begin addressing that issue at the moment.

nucleogenesis · 2023-12-21T21:13:03Z

package.json

@@ -26,7 +26,7 @@
    "app-devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:app-python-devserver yarn:hashi-dev --",
    "devserver": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly {1}\" yarn:python-devserver yarn:hashi-dev --",
    "devserver-hot": "concurrently --passthrough-arguments --kill-others \"yarn:watch-hot --watchonly {1}\" yarn:python-devserver yarn:hashi-dev --",
-    "devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --",
+    "devserver-with-kds": "concurrently --passthrough-arguments --kill-others \"yarn:watch --watchonly={2} --require-kds-path --kds-path={1}\" yarn:python-devserver yarn:hashi-dev --",


Non-blocking as I think this is probably out of scope of the issue you're trying to resolve here, so I'll make a follow-up issue after merging this.

But - I think that we'll need to do some tweaking because when I use the --watchonly flag here with:

yarn devserver-with-kds path/to/kds user_auth then the build system doesn't recognize whenever I make a change in KDS.

But when I don't pass in any plugins for the --watchonly then the front-end rebuilds reactively to changes I make within KDS.

I understand that. And yes, it'd be better to have a follow-up for this issue so that it can be focussed on separately. I too am not sure how to move forward with it. I believe this is one important feature to have and requires separate attention.

Add error message when kds path not specified

74f3799

github-actions bot added DEV: tools Internal tooling for development SIZE: very small labels Dec 16, 2023

nucleogenesis requested changes Dec 18, 2023

View reviewed changes

Change descriptions for errors and flags

51a24ab

github-actions bot added the SIZE: small label Dec 19, 2023

nucleogenesis approved these changes Dec 21, 2023

View reviewed changes

nucleogenesis merged commit e612388 into learningequality:release-v0.16.x Dec 21, 2023
33 checks passed

nucleogenesis mentioned this pull request Dec 21, 2023

devserver-with-kds: The --watchonly flag being stops watching KDS changes #11667

Closed

MisRob mentioned this pull request Feb 15, 2024

Display a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11657

Add a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11657

KshitijThareja commented Dec 16, 2023

github-actions bot commented Dec 16, 2023 •

edited

Loading

KshitijThareja commented Dec 16, 2023

nucleogenesis left a comment

nucleogenesis Dec 18, 2023

nucleogenesis Dec 18, 2023

nucleogenesis Dec 18, 2023

KshitijThareja commented Dec 19, 2023

nucleogenesis left a comment

nucleogenesis Dec 21, 2023

KshitijThareja Dec 22, 2023

Add a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11657

Add a meaningful error warning when KDS path is not provided when running yarn run devserver-with-kds #11657

Conversation

KshitijThareja commented Dec 16, 2023

Summary

References

Reviewer guidance

Screenshot

Testing checklist

PR process

Reviewer checklist

github-actions bot commented Dec 16, 2023 • edited Loading

Build Artifacts

KshitijThareja commented Dec 16, 2023

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Dec 18, 2023

Choose a reason for hiding this comment

nucleogenesis Dec 18, 2023

Choose a reason for hiding this comment

nucleogenesis Dec 18, 2023

Choose a reason for hiding this comment

KshitijThareja commented Dec 19, 2023

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Dec 21, 2023

Choose a reason for hiding this comment

KshitijThareja Dec 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2023 •

edited

Loading