pipalacademy / self-hosting-101

MIT License
1 stars 0 forks source link

Suggestions for restructuring #1

Closed anandology closed 2 years ago

anandology commented 2 years ago

Here are some suggestions to make the code better.

Move core to top-level

Currently, it is organized as:

self-hosting-101
├── self_hosting
│   ├── __init__.py
│   ├── core
│   │   ├── ...
│   └── project.py
└── tasks.yml

My suggestion is to make it this:

self-hosting-101
├── core
│   ├── ...
└── selfhosting.py
└── tasks.yml

Only the two files selfhosting.py and tasks.yml are related to this project and everything else is generic and will be part of core.

If core becomes a new repository or a python package, the structure of self-hosting-101 will stay the same except losing core.

Remove global state

Now that we have an App, make tasks and valiators part of the app rather than keeping them global.

Validator API

The concept of site is not generic enough and I think that should go away.

Also, expecting every Validator to have a title field is not a good API. I think we can use str(validator) to get the title of it.

class check_webpage_content(Validator):
    def __init__(self, url, expected_text):
        self.url = url
        self.expected_text = expected_text

   def __str__(self):
       return f"Check webpage content: {self.url}"

    def validate(self):
        content = requests.get(self.url).text
        if self.expected_text not in content:
            message = f'Text "{self.expected_text}"\nis expected in the web page {self.url},\nbut it is not found.'
            raise ValidationError(message)

I've also replaced the CheckFailed with ValiadtionError.

The do_validate has been renamed to validate, the original validate in that class need to get a new name.

The implementation of the check_webpage_content in Rajdhani was aware of the base_url, which is not possible if we want to be generic. That means we need to specify the entire URL in tasks.yml. Let's see how that task can be specified.

- check_webpage_content:
      url: "{{ config.base_url }}/search"
      expected_text: "<h2>Search Trains</h2>"

We may have to support some kind of Jinja expressions in the tasks.yml, similar how Ansible works.

Core Validators

Add some core validators. These validators will be available for every app and they need not be added explicitly.

Add app.run

Add a run method to the App that would parse the command-line args, does the initialization and start the webserver serving the flask app.

nikochiko commented 2 years ago

url: "{{ config.base_url }}/search"

I don't think we should make the creator think in terms of URLs that change deployment-to-deployment. Validators (once created) should provide the right level of abstraction as per the context. For example for self-hosting, the abstractions should let the creator check for "does this package exist?" or "is this application deployed?". For webapps, for which check_webpage_content is ideal should let the creators think in terms of "does this endpoint serve the correct data?". Computing the exact URLs should be the responsibility of validators.

anandology commented 2 years ago

Fair, but what is the right level of abstraction?

nikochiko commented 2 years ago

I think one at the level of the concepts being taught, without being too specific. Like, if a creator want to teach webapps with a project about X, it should be at the level that it can generalize across different X. For rajdhani for example, the right level of abstraction for checks would be to "check website content", "check creation", "check updation", "check deletion". For a linux/infra course, that could be at the level of packages/files/services that can be used for self-hosting as well as deployment of other apps. For a course on containers relying on docker, that would be at the level of docker.

anandology commented 2 years ago

How about this?

Support adding config to the tasks.yml and pass the config to all the validators. The config support python string substitution and the variable name will be available.

config:
    base_url: https://{name}.rajdhani.pipal.in/
    domain: {name}.selfhosting.pipal.in

This would support all the cases that you mentioned without adding too much complexity. What do you think @nikochiko?

nikochiko commented 2 years ago

Agreed! Seems like a good approach. How about renaming tasks.yml to treadmill.yml now that we have more than just the tasks there?

anandology commented 2 years ago

keep it as tasks.yml for now. We'll get back to the name later.