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: add all default assets to aspects-superset image to avoid load it via larger file #500

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

Henrrypg
Copy link
Contributor

Description

This PR intends to modify aspects-superset image to contains all default openedx-assets and load it from there, and still load custom assets with the patch over superset-extra-assets. This changes will keep a small assets.yaml file and ConfigMap for K8S,

How to test it

You should run an openedx environment from cero without any problem in the init.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 26, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 26, 2023

Thanks for the pull request, @Henrrypg! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@Henrrypg
Copy link
Contributor Author

@Ian2012 Can you please take a look at this?

Copy link
Contributor

@Ian2012 Ian2012 left a comment

Choose a reason for hiding this comment

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

The sql files can be removed from the docker image and can live only in the plugin templates.

Copy link
Contributor

@Ian2012 Ian2012 left a comment

Choose a reason for hiding this comment

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

@bmtcril do you want to take a look?

@pomegranited you can rebase over this changes on your PR

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Just the one question. We should also update documentation to indicate that any time an asset or translation change is made we will need to rebuild the image before re-running init to test now.

@Henrrypg Henrrypg force-pushed the hpg/assets-bug-resolution branch from f6fecc5 to 90903c3 Compare October 31, 2023 20:21
@Ian2012
Copy link
Contributor

Ian2012 commented Oct 31, 2023

@bmtcril why translations?

Those are still loaded from a config file

@bmtcril
Copy link
Contributor

bmtcril commented Oct 31, 2023

@Ian2012 true, but I think the point about assets still stands

@Henrrypg Henrrypg force-pushed the hpg/assets-bug-resolution branch from 93f9614 to a117754 Compare October 31, 2023 20:43
README.rst Outdated Show resolved Hide resolved
@Henrrypg Henrrypg force-pushed the hpg/assets-bug-resolution branch from a117754 to ef520eb Compare October 31, 2023 21:13
@Henrrypg
Copy link
Contributor Author

Henrrypg commented Nov 1, 2023

One test is failing, it seems that aspects-superset image is not being built on workflow, maybe we need to add this step? @Ian2012 @bmtcril

@Ian2012
Copy link
Contributor

Ian2012 commented Nov 1, 2023

@Henrrypg the error is caused by a flaky migration in Superset. The workflow is re run

1 similar comment
@Ian2012
Copy link
Contributor

Ian2012 commented Nov 1, 2023

@Henrrypg the error is caused by a flaky migration in Superset. The workflow is re run

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Bah sorry, I merged another PR before this one. You'll need to rebase and probably move the new files over to the new location. This looks ready to merge once that's done, though.

@Henrrypg Henrrypg force-pushed the hpg/assets-bug-resolution branch from ef520eb to 1d017c2 Compare November 1, 2023 14:54
@bmtcril bmtcril merged commit cb48cf8 into openedx:main Nov 1, 2023
5 checks passed
@openedx-webhooks
Copy link

@Henrrypg 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants