search

overhangio / tutor-discovery

Course Discovery plugin for Tutor
GNU Affero General Public License v3.0
11 stars 40 forks source link

Error on running cache_programs command. #40

Closed Faraz32123 closed 9 months ago

Faraz32123 commented 1 year ago

When I run the cache_programs command, I faced the following error although the program is syncing to my LMS.

Screenshot 2023-06-05 at 2 25 13 PM Screenshot 2023-06-05 at 2 25 58 PM Screenshot 2023-06-05 at 2 26 18 PM
Faraz32123 commented 1 year ago

Hi @regisb

I have investigated the error and there is no issue.

When we run tutor local launch, it will be retrieving pathways/programs from both http://discovery.local.overhang.io/api/v1/ and http://discovery.local.overhang.io:8381/api/v1/. So while accessing dev url with port 8381 on tutor local launch, error will occur. When we run tutor dev launch, it will be retrieving pathways/programs from both http://discovery.local.overhang.io/api/v1/ and http://discovery.local.overhang.io:8381/api/v1/. So while accessing local url without any port on tutor dev launch, error will occur.

Why it hits both local and dev urls? because in by default configurations of sites models, we have two instances of both local and dev sites. So according to code it will hit all objects of sites models.

Screenshot 2023-06-05 at 8 21 04 PM
regisb commented 1 year ago

The cache_programs command will cache programs for all Site instances, and there is no option to run it for a single site. Thus, the instructions from the plugin readme will always result in an error. I think this should be considered as an issue. It can be resolved in one of two ways:

  1. Add a --site=... option to the cache_programs command in edx-platform. If unspecified, the command will run for all sites (current behaviour). Then, modify the instructions in this plugin to make use of that new --site option.
  2. Modify the docs of the tutor-discovery plugin to explain that the command will always result in an error, and that we are quite sorry about it...

Of course, option 1 is best, but it requires more work. What do you think @Faraz32123?

Faraz32123 commented 1 year ago

The cache_programs command will cache programs for all Site instances, and there is no option to run it for a single site. Thus, the instructions from the plugin readme will always result in an error. I think this should be considered as an issue. It can be resolved in one of two ways:

  1. Add a --site=... option to the cache_programs command in edx-platform. If unspecified, the command will run for all sites (current behaviour). Then, modify the instructions in this plugin to make use of that new --site option.
  2. Modify the docs of the tutor-discovery plugin to explain that the command will always result in an error, and that we are quite sorry about it...

Of course, option 1 is best, but it requires more work. What do you think @Faraz32123?

Yes, you are right, option 1 is best. I will then go for option 1 to fix this issue.

Faraz32123 commented 1 year ago

The cache_programs command will cache programs for all Site instances, and there is no option to run it for a single site. Thus, the instructions from the plugin readme will always result in an error. I think this should be considered as an issue. It can be resolved in one of two ways:

  1. Add a --site=... option to the cache_programs command in edx-platform. If unspecified, the command will run for all sites (current behaviour). Then, modify the instructions in this plugin to make use of that new --site option.
  2. Modify the docs of the tutor-discovery plugin to explain that the command will always result in an error, and that we are quite sorry about it...

Of course, option 1 is best, but it requires more work. What do you think @Faraz32123?

Hi @regisb , I have created a PR against edx-platform for this fix. I will update its readme in a while.

regisb commented 1 year ago

Awesome! Great job.

Faraz32123 commented 1 year ago

@regisb , fix PR has been merged into edx-platform master branch, issue has been fixed. So, closing this issue.

regisb commented 1 year ago

:thinking: unless I'm mistaken, the tutor-discovery users still face an error when running the instructions from the README, so I think this issue should remain open. We should do the following:

  1. Open a PR to backport the change to the open-release/palm.master branch
  2. Until the PR is merged and palm.2 is released, we should cherry-pick the change in the tutor "openedx" Docker image.
  3. ~We should modify the instructions in the tutor-discovery README to actually use that new option.~ EDIT: scratch that, the instructions have already been updated.

What do you think?

Faraz32123 commented 1 year ago

🤔 unless I'm mistaken, the tutor-discovery users still face an error when running the instructions from the README, so I think this issue should remain open. We should do the following:

  1. Open a PR to backport the change to the open-release/palm.master branch
  2. Until the PR is merged and palm.2 is released, we should cherry-pick the change in the tutor "openedx" Docker image.
  3. ~We should modify the instructions in the tutor-discovery README to actually use that new option.~ EDIT: scratch that, the instructions have already been updated.

What do you think?

@regisb , I have created a PR against edx-platform open-release/palm.master branch. Until this PR is not merged, I have created a PR to tutor to add a change in the tutor "openedx" Docker image, users will not face any error after this.