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

integrate coredev.buildout (was PR #1670) #1671

Merged
merged 79 commits into from
Jul 20, 2024

Conversation

stevepiercy
Copy link
Contributor

@stevepiercy stevepiercy commented Jun 19, 2024

@stevepiercy stevepiercy self-assigned this Jun 19, 2024
…d with a MyST extension we use), thus simplifying the diff review against `6.0`.
…'s not an explanation or reference.

- Purge git and GitHub material, and refer to first-time contributors.
- Modify pre-requisites
@stevepiercy
Copy link
Contributor Author

I could really use a review from anyone who cares about how to contribute to and develop in Plone core of just this one file (https://plone6--1671.org.readthedocs.build/contributing/core/index.html) in this commit: e2fb4f8 @jensens @petschki @ericof @MrTango @sneridagh @davisagli @thet This one file is the key to getting new suckers contributors to Plone core.

For anyone who cares about releasing Plone, specifically @mauritsvanrees, I especially would like a review on https://plone6--1671.org.readthedocs.build/contributing/core/release.html

More to come, but these two pages are super critical to the others.

@petschki
Copy link
Member

petschki commented Jun 20, 2024

When it comes to "Edit packages" I always create a local.cfg ( -> ignored in .gitignores ) which looks like this:

[buildout]
extend = 
    core.cfg

auto-checkout +=
    plone.app.event
    icalendar
    my.package

[sources]
# add sources here, which arent in the sources.cfg already or need another branch
my.package = git [email protected]:collective/my.package.git branch=main

and then run

$ ./bootstrap.sh -c local.cfg

so you do not have to edit any of the original files ( which might also get manipulated by mr.roboto ... eg. checkouts.cfg )

@stevepiercy
Copy link
Contributor Author

stevepiercy commented Jun 20, 2024

so you do not have to edit any of the original files ( which might also get manipulated by mr.roboto ... eg. checkouts.cfg )

@thet also suggested something similar.

buildout.local.cfg

[buildout]
extends =
  buildout.cfg

auto-checkout =
    # Add packages which you want to develop
    plone.app.caching
    plone.caching
    plone.app.event
    icalendar
    # others
    ...

However further on, according to this ancient documentation, the release manager would look in checkouts.cfg to see what needs to be released. Additionally Jenkins looks at this file—and not any custom file—to run the full test suite. Is all that correct?

It would be good to verify what is the actual preferred process to contribute to Plone core.

@davisagli davisagli dismissed their stale review July 1, 2024 04:57

the changes I requested were done but I didn't have time to review the other parts of the PR yet

@stevepiercy
Copy link
Contributor Author

@davisagli I would like to merge this after the Volto Team meeting on Tuesday, July 2. Will that work for you?

@jensens
Copy link
Member

jensens commented Jul 1, 2024

Sorry for the delay, I'll look into it ASAP.

"keywords": "Plone, continuous integration, best practices"
---

# Essential continuous integration practices
Copy link
Member

Choose a reason for hiding this comment

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

This whole page is outdated, it does not mention mr.roboto, GHA nor plone/meta which make almost all the rules described outdated.

Sorry that it took me forever to react on the call to help on this PR 🙇🏾

I can do the update, should I push it directly to this branch? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gforcada please create a PR against this branch for review. Do not push commits directly to this branch.

I'm not clear on which repositories plone/meta applies. From what I know, many repositories use make and not tox, including Volto, plone.restapi, and Documentation. Until there is both agreement and implementation for a unified approach, I don't know whether plone/meta should be declared as authoritative for all projects under the Plone GitHub organization.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks I will do that (the branch).

As for plone/meta or make, well, it's literally all but volto related packages that already use plone/meta. There are a few that are missing (like CMFPlone) that I did not had the time to apply plone/meta to it.

But this documentation is meant for classic UI, or? 🤔 I meant I saw quite a few references that always branch to say "if you are doing volto then see ../volto"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the documentation is intended for anything that is not Volto, which includes Classic UI, core add-ons, and other core packages.

There is also an effort with cookiecutter and mxdev, which is intended to replace buildout, at which point another rewrite of docs will need to take place.

I don't know how that reconciles with plone/meta. I just document what I understand.

Copy link
Member

Choose a reason for hiding this comment

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

plone/meta and mxdev/cookiecutter are orthogonal, at least I'm 100% sure with mxdev, with cookiecutter that depends on what's being done, of for which purpose is used 😓

I've seen so many cookieXX repositories, that I'm no longer sure what one should use 🤷🏾

but if the goal is to replace zc.buildout, then plone/meta has nothing to do with that and should be fine to document.

Actually plone/meta has quite some exhaustive documentation on its own 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gforcada even before your review and subsequent pull request, I had a lot of issues with this document. It never had a high-level review. I apologize for not bringing up my concerns earlier, but here they are now.

  1. It's not clear what is the purpose of this document.
  2. It's not clear who is the target audience.
  3. Is it a tutorial, a how-to guide, an explanation, or a reference? See https://diataxis.fr/ for details of what these categories mean.
  4. Much of the content is redundant to existing parts of the documentation, as well as within this pull request, including:

Once we have an idea of what we want to do, then we can rewrite this document to achieve that goal. I'd be happy to have a chat with you in real-time in Discord, or continue the conversation here.


For cookie*, the primary repo is https://github.com/plone/cookieplone. It uses templates found in https://github.com/plone/cookieplone-templates. I don't know whether that repo uses plone/meta itself, but its templates do.

cookie* uses mxdev, as described in https://6.docs.plone.org/install/manage-add-ons-packages.html#manage-plone-backend-packages-with-mxdev.


Are these files plone/meta's documentation?

Copy link
Member

Choose a reason for hiding this comment

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

In reverse order 🙃

  • yes, that's the documentation for plone/meta

  • oh nice, thanks for the cookie pointers, now it's a bit more clear to me, the (not to be answered and solved now) is how that works with mr.bob and bobtemplates.plone and all that ecosystem, or again, we have cookie* for volto and mr.bob for classic UI? 🤔

  • for this document, I would say that it's an explanation of how the CI system works, being the target audience newcomers.

With the links you provided (thanks!) I would say it could be linked from https://6.docs.plone.org/contributing/index.html#continuous-integration, then, we could simplify my PR to remove the explanation about PRs and be more focused only on the tooling involved.

How that sounds?

As for talking in discord, sure, this week, I do have time in European evenings, something like https://www.timeanddate.com/worldclock/converter.html?iso=20240708T170000&p1=37&p2=202 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • OK on meta docs.

  • I encourage you to try cookieplone for fun. If there is something missing, or is not clear how to do, then file an issue.

  • I think an explanation page would be good. I do not know all the parts of CI, what each part does, or how they al work together to fulfill specified tasks.

    In that context, let's change the document title to "Continuous integration". If we keep "essential" or "practices", then we start telling the reader how to do things or what things they should do, which makes it a how-to guide, not an explanation.

    Also if you use Mermaid for diagrams, you could even draw a picture of the workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gforcada to get this PR merged, I will remove the CI stuff entirely. It's not correct and should not be published in its current state. The rest of it is good. We can add the CI stuff in another PR.

I'll work on pulling it out of this PR, and moving it back to the coredev directory to preserve the grammar and MyST work I did. I'll do that sometime this week.

Co-authored-by: Gil Forcada Codinachs <[email protected]>
tisto
tisto previously approved these changes Jul 14, 2024
Copy link
Member

@tisto tisto left a comment

Choose a reason for hiding this comment

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

LGTM!

@stevepiercy
Copy link
Contributor Author

Thanks everyone! Merging!

@gforcada let's work on the CI page when you get the chance.

@stevepiercy stevepiercy merged commit 12bf1cc into 6.0 Jul 20, 2024
3 checks passed
@stevepiercy stevepiercy deleted the integratel-coredev.buildout-2 branch July 20, 2024 12:31
@mauritsvanrees
Copy link
Member

Thanks for pushing this, Steve!

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