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

alembic configuration story needs to improve #10053

Closed
abitrolly opened this issue Sep 18, 2021 · 8 comments · Fixed by #12792
Closed

alembic configuration story needs to improve #10053

abitrolly opened this issue Sep 18, 2021 · 8 comments · Fixed by #12792
Labels
developer experience Anything that improves the experience for Warehouse devs feature request

Comments

@abitrolly
Copy link
Contributor

What's the problem this feature will solve?

Fixing linting errors after alembic is easy, but a time consuming distractions. While alembic doesn't produce linter friendly output by default, it allows to run write hooks. https://alembic.sqlalchemy.org/en/latest/autogenerate.html?highlight=black#applying-post-processing-and-python-code-formatters-to-generated-revisions

Describe the solution you'd like

I would like autogenerated migrations to be black and isort valid.

Additional context

I could not find where Pyramid stores configuration for alembic, so I can't solve this issue right now.

Long story sqlalchemy/alembic#925

@ewjoachim ewjoachim added the developer experience Anything that improves the experience for Warehouse devs label Sep 21, 2021
@ewjoachim
Copy link
Contributor

Hey, thanks for opening the issue.

For the record, I'm not entirely comfortable with how adversarial the conversation went on the alembic discussion you linked. Given an upstream tool provider has already implemented everything we need to get it done easily on our side, I don't think it's fair to request that our own usecase would become the new default for everyone. The Python world is large, the practices are equally diverse, and when a project like Alembic has made a choice to provide a hook and not "just" lint by default, it's rarely an oversight. Especially when they explained the reasoning behind this choice, I'm not sure there was a need to continue insisting. I can see how the Alembic people might not have gotten a good experience of this interaction. PyPI is an important project among the community, and we ought to project the best of ourselves to other people when interacting with them in the name of this project. Keep in mind that we're (mostly) all volunteers here with limited time to tackle interesting problems.

With that said, I believe alembic configuration is dynamic, not static (thus why we can't find a .ini file).
I guess this should give us enough to work with:
https://github.com/pypa/warehouse/blob/781e4ae7254f0cc30f868f149e23393842eb0a79/warehouse/db.py#L27-L31

@ewjoachim
Copy link
Contributor

ewjoachim commented Sep 21, 2021

I believe this would look something like:

hooks = {
    "black": {"options": "..."},
    "isort": {"options": "..."},
}

alembic_cfg.set_section_option(section="post_write_hooks", name="hooks", value=",".join(hooks))
for key, config in hooks.items():
    config["type"] = "console_scripts"
    config["entrypoint"] = key
    for name, value in config.items():
        alembic_cfg.set_section_option(section="post_write_hooks", name=f"{key}.{name}", value=value)

or alternatively:

alembic_cfg.set_section_option(section="post_write_hooks", name="hooks", value="black,isort")
alembic_cfg.set_section_option(section="post_write_hooks", name="black.type", value="console_scripts")
alembic_cfg.set_section_option(section="post_write_hooks", name="black.entrypoint", value="black")
alembic_cfg.set_section_option(section="post_write_hooks", name="black.options", value="...")
alembic_cfg.set_section_option(section="post_write_hooks", name="isort.type", value="console_scripts")
alembic_cfg.set_section_option(section="post_write_hooks", name="isort.entrypoint", value="isort")
alembic_cfg.set_section_option(section="post_write_hooks", name="isort.options", value="...")

@abitrolly
Copy link
Contributor Author

I didn't like the way the report went myself, but I hope we cleared misunderstandings in the process. black insane popularity is not because there were no linters, or because the linters were of poor quality. One day I will a better way to explain that default experience matter for occasional contributors, such as those coming from Octoberfest. Then people probably won't close such issues as not important. Adding to that, I opened PyCQA/isort#1812 as a practical way to gather data about what the defaults should be.

With that said, I believe alembic configuration is dynamic, not static (thus why we can't find a .ini file).

Is it possible to point it to the warehouse/migrations/alembic.ini?

@ewjoachim
Copy link
Contributor

Is it possible to point it to the warehouse/migrations/alembic.ini?

I don't understand your suggestion.

I'd say either we need to have all the configuration in an ini file, or none. My suggestion was if we went towards none. I don't know what will need to change for us to go to "all".

@abitrolly
Copy link
Contributor Author

I'd actually prefer all Alembic config to be in alembic.ini, because it is the scenario covered in its documentation. From a DevOps standpoint it is not a good practice to hardcode config for invocation of additional shell tools in the code.

@ewjoachim
Copy link
Contributor

Well, you'll have to figure out how to link to the database url, which is a project setting, controllable by an environment variable.

@abitrolly
Copy link
Contributor Author

Alembic allows to merge configs, but it doesn't report if it fails to find the config file - sqlalchemy/alembic#931 So far I couldn't make it work.

abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 7, 2021
The `url` setting was unused - it should be `sqlalchemy.url`, and
the DB is set up in `env.py`.
@abitrolly
Copy link
Contributor Author

I am trying to use warehouse.config directly in env.py, but tests keep failing. I do not understand how conftest.py mocks config object.

abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 9, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 9, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 9, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 9, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 9, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 12, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 12, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 12, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 13, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 13, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 13, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 25, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 25, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 25, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 28, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 28, 2021
abitrolly added a commit to abitrolly/warehouse that referenced this issue Oct 28, 2021
miketheman added a commit to miketheman/warehouse that referenced this issue Jan 9, 2023
After a migration file is generated, run `black` and `isort` to conform
to our linter rules.

Closes pypi#10053
Closes pypi#10146

Signed-off-by: Mike Fiedler <[email protected]>
ewdurbin pushed a commit that referenced this issue Jan 9, 2023
After a migration file is generated, run `black` and `isort` to conform
to our linter rules.

Closes #10053
Closes #10146

Signed-off-by: Mike Fiedler <[email protected]>

Signed-off-by: Mike Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer experience Anything that improves the experience for Warehouse devs feature request
Projects
None yet
2 participants