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

Config with default template #37

Merged
merged 11 commits into from
Mar 14, 2023
Merged

Conversation

gforcada
Copy link
Sponsor Member

Dependency to CMFPlone and p.a.layout can be removed thanks to plone.base 🎉

Unfortunately tests do fail because the recent move of getNavigationRoot to plone.base is still not released and we don't have a way to specify in a repository to use a checkout of another repository.

Dependencies are all declared but the one to plone.app.vocabularies, as that's only being used in:

    uid = schema.Choice(
        title=_("Target collection"),
        description=_("Find the collection which provides the items to list"),
        required=True,
        vocabulary="plone.app.vocabularies.Catalog",
    )

And adding will certainly generate plenty of circular imports 😓

@mister-roboto
Copy link

@gforcada thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@jensens
Copy link
Sponsor Member

jensens commented Mar 13, 2023

vocabulary="plone.app.vocabularies.Catalog",

I would ignore it. In tests this can be mocked. Long term plan is to have portlets as a core-addons for classicui and move all portlet-code above CMFPlone.

@jensens
Copy link
Sponsor Member

jensens commented Mar 13, 2023

@jenkins-plone-org please run jobs

Copy link
Sponsor Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Unfortunately tests do fail because the recent move of getNavigationRoot to plone.base is still not released and we don't have a way to specify in a repository to use a checkout of another repository.

mxdev to the rescue! But then we need to undo this a bit later once it is no longer needed.

It would be better to make a release of plone.base. One of us could do that once Jenkins is green again and has caught its breath.
We have gotten way too many checkouts, especially this weekend, so we should do some more releases.

And I would need to update the plone.base version in https://dist.plone.org/release/6.0-dev/constraints.txt

@mauritsvanrees
Copy link
Sponsor Member

Dependencies are all declared but the one to plone.app.vocabularies (...) And adding will certainly generate plenty of circular imports

Wait, you say it is not declared, but it is. It was already declared in setup.py on master, and that is still true on this branch.

Since nothing is changed, it should be okay. And this will live above CMFPlone in the future.

@mauritsvanrees
Copy link
Sponsor Member

Hehe, this PR adds a dependency on plone.app.collection. No on noticed, including me. :-D

Jenkins noticed though:

Got plone.app.collection 2.0b5.
While:
  Installing instance.
  Getting distribution for 'plone.app.collection'.
Error: Picked: plone.app.collection = 2.0b5

Lemme fix that.

Also removed the dependency on plone.app.contenttypes, though this is still in the test requirements.
@mauritsvanrees
Copy link
Sponsor Member

Jenkins is green, but a review of my last commit would be good.

@jensens
Copy link
Sponsor Member

jensens commented Mar 14, 2023

LGTM

@jensens jensens merged commit d081c75 into master Mar 14, 2023
@jensens jensens deleted the config-with-default-template-b1750d51 branch March 14, 2023 17:04
@gforcada
Copy link
Sponsor Member Author

@mauritsvanrees thanks for the fixes!

It would be great if z3c.dependencychecker does notice that an import is within a try/except block 🤔

@davisagli
Copy link
Sponsor Member

we don't have a way to specify in a repository to use a checkout of another repository

Have you discussed a preferred way to add support for this yet?

I would recommend avoiding anything that involves putting this information in files in the repository (because then it has to be removed again when the PRs are merged). My suggestion would be that we invent a syntax for putting a list of connected PRs in the PR description, and then make the CI smart enough to find them there and use them.

@gforcada
Copy link
Sponsor Member Author

@davisagli we were thinking of using mxdev, almost sure for buildout.coredev for single packages might be a good solution as well, though more and more files cluttering the top level folder is a bit annoying TBH.

Your idea sounds great, but too magic 🪄 ? 🤔 I guess we can inspect the comments of a PR within a GHA? That sounds like an external GHA that we add to all repositories workflows? Might be worth investigating 👍🏾

If we do changes incrementally (i.e. that PRs can be merged one at a time keeping Jenkins always green) then having always the dependencies checked out might do the trick as well and avoid the extra GHA.

For PRs depending on other PRs that are not done in an incremental way, then we do need this GHA...

@mauritsvanrees
Copy link
Sponsor Member

Locally tox -e test should also work, and that cannot look in a comment on GH.

@davisagli
Copy link
Sponsor Member

@mauritsvanrees I'm not sure "cannot" is correct, but I see your point.

@gforcada
Copy link
Sponsor Member Author

Indeed, very good point, locally though, we can always say that you can use a buildout.coredev checkout and have the distributions and branches sorted out at that level, or? 🤔

That's what we have been doing until now at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants