Skip to content

Conversation

@syedimranhassan
Copy link
Contributor

Please consider the following when opening a pull request:

  • Link to the relevant JIRA ticket(s) and tag any relevant team(s).
  • Squash your changes down into one or more discrete commits.
    In each commit, include description that could help a developer
    several months from now.
  • If running make upgrade, run as close to the time of merging as possible
    to avoid accidentally downgrading someone else's package.
    Put the output of make upgrade in its own separate commit,
    decoupled from other code changes.
  • Aim for comprehensive test coverage, but remember that
    automated testing isn't a substitute for manual verification.
  • Carefully consider naming, code organization, dependencies when adding new code.
    Code that is amenable to refactoring and improvement benefits all platform developers,
    especially given the size and scope of edx-platform.
    Consult existing Architectural Decision Records (ADRs),
    including those concerning the app(s) you are changing and
    those concerning edx-platform as a whole.

@syedimranhassan syedimranhassan force-pushed the ihassan/OPS-3772_clean_up_rendering_code branch 5 times, most recently from d174694 to b8a31b9 Compare October 3, 2019 13:45
@coryleeio
Copy link
Contributor

coryleeio commented Oct 3, 2019

This looks good in theory to me, but you will need to sort out why the python tests are failing.
Theoretically since we always run everything with ansible these defaults should have always been in use. It is possible the testing environment doesn't use the ansible defaults, in which case we may have to either set the values in the envs/test.py in the respective apps (or whatever environment it is using)
OR we could drop a few of the ansible defaults back to the application defaults (because we'll have the rendered value in edx-internal/edx-remote-config so we could just not use the ansible defaults for those values)

@coryleeio
Copy link
Contributor

It is also possible some values were from the common config and need to be dropped into both the lms/envs/common.py and the cms/envs/common.py

@coryleeio
Copy link
Contributor

I can dig deeper on this if you are blocked but i assume you're still iterating so I'll leave it for now

# 'ENABLE_CORS_HEADERS': False,
# 'ENABLE_CROSS_DOMAIN_CSRF_COOKIE': False,
# 'ENABLE_COUNTRY_ACCESS': False,
# 'ENABLE_CREDIT_API': False,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add these back if we can get them working with the tests passing.

# Note: Ensure 'CUSTOM_COURSE_URLS' has a matching value in lms/envs/common.py
'CUSTOM_COURSE_URLS': False
'CUSTOM_COURSE_URLS': False,
# 'DASHBOARD_FACEBOOK': False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here - if they're set in configuration can we add them here or were they commented because they cause the tests to fail (Perhaps this is just a WIP?)

Copy link
Contributor

@coryleeio coryleeio left a comment

Choose a reason for hiding this comment

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

I see you commented a bunch of variables - I suspect this is for getting the tests passing, but we'll want to make sure they make it in here if they aren't overwritten in prod, as otherwise we'll be making configuration changes which we definitely want to avoid. In general this looks like the right idea though.

@coryleeio
Copy link
Contributor

thumbed so you can move forward but I assume you'll keep iterating on the tests for a bit. Please keep us posted on when this will go out so we can monitor it closely.

@syedimranhassan syedimranhassan force-pushed the ihassan/OPS-3772_clean_up_rendering_code branch 4 times, most recently from fafd239 to 669b8b3 Compare October 15, 2019 14:20
@syedimranhassan syedimranhassan changed the title Move lms/cms defaults from configration repo Move lms defaults from configration repo Oct 16, 2019
@syedimranhassan syedimranhassan force-pushed the ihassan/OPS-3772_clean_up_rendering_code branch 2 times, most recently from d5ef191 to bfb4137 Compare October 16, 2019 12:48
@syedimranhassan syedimranhassan force-pushed the ihassan/OPS-3772_clean_up_rendering_code branch from bfb4137 to 549079b Compare October 17, 2019 06:25
@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@syedimranhassan syedimranhassan merged commit e861b88 into master Oct 17, 2019
@syedimranhassan syedimranhassan deleted the ihassan/OPS-3772_clean_up_rendering_code branch October 17, 2019 07:25
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, October 17, 2019.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants