Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

edx_notes_api config for native image #993

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

Conversation

zubairshakoorarbisoft
Copy link
Contributor


I've completed each of the following or determined they are not applicable:

  • Made a plan to communicate any major developer interface changes (or N/A)

RESULTS_DEFAULT_SIZE: 25
RESULTS_MAX_SIZE: 250
SECRET_KEY: CHANGEME
USERNAME_REPLACEMENT_WORKER: OVERRIDE THIS WITH A VALID USERNAME
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -390,6 +390,7 @@ services:
- lms
- mysql57
image: edxops/notes:${OPENEDX_RELEASE:-latest}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to use new Ansible free image here and test it before we merge this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please help me to find it how you have done it before for other PRs?
I followed analytics PR, so is there anything which I'm missing from analytics PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to change the image specified here currently with the new Ansible free image once we merge the PR in notes-api repository

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image name changed

@@ -407,6 +408,9 @@ services:
ENABLE_DJANGO_TOOLBAR: 1
ELASTICSEARCH_URL: "http://edx.devstack.elasticsearch710:9200"
ELASTICSEARCH_DSL: "http://edx.devstack.elasticsearch710:9200"
volumes:
- /edx/var/edx_notes_api/
Copy link
Contributor

Choose a reason for hiding this comment

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

This wasn't present before. We shouldn't add anything not related to our work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't add it as part of my PR. We simply don't need it for notes-api if it wasn't specified already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, verified it. It was not there before.

@@ -453,6 +453,9 @@ dev.shell.analyticsapi:
dev.shell.insights:
docker-compose exec insights env TERM=$(TERM) bash -c 'eval $$(source /edx/app/insights/insights_env; echo PATH="$$PATH";) && /bin/bash'

dev.shell.edx_notes_api:
Copy link
Contributor

@aht007 aht007 Dec 9, 2022

Choose a reason for hiding this comment

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

If there is no specific make target for note_api then according to my understanding this command will be executed which means there should be no need to add this command explicitly. Can you please try this theory too when testing the Ansible free image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants