search

eduNEXT / tutor-contrib-edunext-distro

A tool to facilitate the customization of an Openedx instance, adding commands and settings to have an easy-to-use and a ready-to-deploy in local or in development openedx distribution.
2 stars 1 forks source link

(DS-536) refactor!: remove distro defaults settings to add from strain/conf file #46

Closed JuanDavidBuitrago closed 1 year ago

JuanDavidBuitrago commented 1 year ago

Description

This PR remove Distro defaults settings to have a Tutor plugins flexible for different users, The principal idea is that people use Distro but with the packages that want to use, making the settings in a config/strain file.

For example, our Olmo master version is set in strain.yml file and contain Django plugins/Themes that use our SAAS. Here are the settings that were removed and added in strain file: https://github.com/eduNEXT/ednx-strains/pull/46/files

How to test

Using stack builder in your local to build the image

  1. Make a strain.yml in a new work directory, use this strain.yml
  2. Inside the work directory, run stack strain create command
  3. Next run stack strain local configure -s install_plugins -s enable_plugins -s save_config -s extra_commands
  4. After this run stack strain local build openedx --no-cache
  5. You can see the image was build correctly!

Using Picasso to build the image

  1. Go to Picasso-strain-builder
  2. In STRAIN_REPOSITORY_BRANCH set jdb/add_default_settings_to_distro, it's the branch where are the settings to Distro
  3. In STRAIN_PATH set olmo/master, it's the strain that has the Distro settings
  4. Click on Build button
  5. You can see the image was build correctly!
Alec4r commented 1 year ago

@JuanDavidBuitrago this looks good for me, just a comment, for next PRs, please split the CI and new feature in differents PRs and I have a question, are there other strains using olive or olmo?

Also, what will happend if my config.yml doesn't have DISTRO_EXTRA_MIDDLEWARES or the others old default variables? will it work?

JuanDavidBuitrago commented 1 year ago

@JuanDavidBuitrago this looks good for me, just a comment, for next PRs, please split the CI and new feature in differents PRs and I have a question, are there other strains using olive or olmo?

@Alec4r I split the CI commit, is in this PR. There are other strains, you can see here, but this all have the settings to continue working as expected.

Also, what will happend if my config.yml doesn't have DISTRO_EXTRA_MIDDLEWARES or the others old default variables? will it work?

I fixed all problems when the defaults variables aren't defined, so if a user doesn't need to use all of them, Distro continue working normally

Can you check again?, please

dcoa commented 1 year ago

We should update the documentation to remove all references to default values.

JuanDavidBuitrago commented 1 year ago

Hi @Alec4r @dcoa

Could you check the new changes please?

JuanDavidBuitrago commented 1 year ago

Sorry for the late review and many comments; this PR is a big breaking change, and I believe it is so important that the users of Distro understand what is happening. I would like you to change a little bit the description of what the Distro is and what this plugin does after this PR and try to adapt the Readme to be easy to know what the user can do; for that reason, I let you several comments around to drop the default explanation and go straight to how to do the things they need to do.

I have a little question, when we merge this, are we going to have an "Olmo" version of the Distro with this plugin in v16? When we migrate a palm, are we planning to stay in v16?

The rest looks good.

I don't think the changes that were made drastically change the description of the Distro. It's just that now we're not going to have Django plugins and Themes by default, now users are free to use whatever they want in their environment.

Regarding the version, this continues to lead to the nomenclature according to the version of Tutor, as it has been working up to now. Remember that in Nutmeg (v14) we went from a v3.2.0 Tag to v14.2.0 just to be consistent with Tutor versions.

JuanDavidBuitrago commented 1 year ago

@MaferMazu Let me know if anything else bothers you, so I can clear it up.

MaferMazu commented 1 year ago

Yes, I really don't understand.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

JuanDavidBuitrago commented 1 year ago

Yes, I really don't understand.

* If we are going to follow the Tutor version this change will create the v15.2.0 being a breaking change? (My proposal around this is to start this breaking change for Palm, and update the release to v16)

* How is not a default having code about eox-theming? (Probably my concern here is because I don't understand what will be the purpose of this plugin)

* In the readme said:

Distro is an opinioned openedx distribution with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution. This can be watch like a tutor-plugin but is taken a little bit far away.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

* And I do not agree with maintaining old documentation that is not consistent with the real behavior. (Adding comments for future behavior or past behavior is okay, but having old documentation with the current behavior in comments ... is not intuitive)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

Hi @Alec4r, could you give me a hand here? Please

Alec4r commented 1 year ago

Yes, I really don't understand.

  • If we are going to follow the Tutor version this change will create the v15.2.0 being a breaking change? (My proposal around this is to start this breaking change for Palm, and update the release to v16)
  • How is not a default having code about eox-theming? (Probably my concern here is because I don't understand what will be the purpose of this plugin)
  • In the readme said:

Distro is an opinioned openedx distribution with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution. This can be watch like a tutor-plugin but is taken a little bit far away.

And If I see Distro as distribution, yes. But this plugin with these changes won't be the opinionated openedx distribution, maybe a tool we use to make easier our work (we are going to have our tools), but not the opinionated distribution. (Probably is a good idea to add some information more accurate about what this plugin really will do)

  • And I do not agree with maintaining old documentation that is not consistent with the real behavior. (Adding comments for future behavior or past behavior is okay, but having old documentation with the current behavior in comments ... is not intuitive)

Maybe I need to understand better what we want to do with this plugin, for that reason, I am not feeling comfortable giving a review.

We have already talk about this, but I could explain again:

Distro is an opinioned openedx distribution.: Yes, The most important part is "is an opinioned openedx distribution", distro is just a number of requirements, settings, xblocks, that edunext considerate are the best, but is just an opinion, so we will move that to an one pager, documentation and strain, due to it's just an opinion, it's similar to NELP opinion, Pearson Opinion, are just opinions.

with some custom stuff to have an easy-to-use and a ready to deploy in local or in development openedx distribution: Second part, how to use this, ok, the answer for this part is "Distro plugin/Tutor Stack Plugin" that help us to load opinions (Strains) easier, but it's just another way to do it, because you can apply NELP opinion without use a plugin, you could use inline plugins, and you can configure each theme or private requirements without need "Distro plugin/Tutor Stack Plugin", but maybe with this plugin is easier than replicate each step.

Why on this version and not wait for palm?, because we need to test now, and be safe all is working while we can change between versions and not in palm while we are in the middle of a big migration.

If you want to see this like "Distro Plugin v2.0" it's ok, this is a big rework of our old plugin.

And of course this will need new documentation, I totally agree with you.

Now, this is the reason why we used to have difference version and we couldn't use just "Tutor Semantic Version", because our break change doesn't depends of tutor version, this is a tool more than a tutor plugin, we are just using tutor technologies for make easier our work.

Alec4r commented 1 year ago

@JuanDavidBuitrago did you already do some test using picasso? could you give me the link or number of the build?

JuanDavidBuitrago commented 1 year ago

@JuanDavidBuitrago did you already do some test using picasso? could you give me the link or number of the build?

Yes @Alec4r, I did the build with this olmo strain and the job in Picasso is #121. This is the Docker Image

Let me know if you need anything else.