-
Notifications
You must be signed in to change notification settings - Fork 4
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
WCM-167: add form for manual multi publish inside vivi #971
base: main
Are you sure you want to change the base?
Conversation
66c3c1c
to
f31f2a8
Compare
core/src/zeit/cms/workflow/cli.py
Outdated
@@ -95,18 +57,19 @@ def publish(): | |||
).start() | |||
|
|||
log.info('Ignoring services %s', options.ignore_services) | |||
zeit.workflow.publish_3rdparty.PublisherData.ignore = options.ignore_services + IGNORE_SERVICES | |||
zeit.workflow.publish_3rdparty.PublisherData.ignore = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Das wird so leider nicht gehen. Das ist ja aktuell (aus Bequemlichkeit) eine prozess-globale Variable, was wir uns leisten konnten, weil es immer nur in dem einzelne CLI-Prozess überhaupt angefasst wurde, weil dessen Lebensdauer isoliert und begrenzt war. Wenn das jetzt aber auch in einem Celery-Kontext klappen soll, müssen wir uns vmtl was anderes ausdenken, wo wir uns das hinspeichern, sodass es genau nur für den einzelnen IPublish.publish()
Aufruf zur Anwendung kommt. (Denn selbst wenn man sich das zurechtmauscheln würde nach dem Motto, die manual
Queue darf halt immer nur 1 Worker haben, dann kann es nicht übersprechen, müsste man mindestens noch sicherstellen, dass es nach Ende des Tasks sicher wieder zurückgesetzt wird -- aber in der Richtung liegen schwer zu debuggende Bugs, insofern würde ich stark davon abraten, das zu verfolgen.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noch schlimmer ist es allerdings bei den ganzen ZCA- oder gar mock-basierten Patches, die wir da vornehmen. Das ignore_services
kann man sich ja vmtl noch irgendwie vorstellen, zb auf die jeweilige Instanz(!) des IPublish
Adapters draufschreiben, und von dort irgendwie(tm) an IPublisherData
weitergeben.
Wir hatten das an ner anderen Stelle schonmal, dass wir eine Custom ZCA-Registry "eingeschoben" haben, sowas müsste man hier dann vmtl auch tun.
title=_('Skip TMS enrich, e.g. checkin already happened'), default=False | ||
) | ||
dlps = zope.schema.Bool(title=_('Update date_last_published_semantic timestamp'), default=False) | ||
# newline separated list of unique ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, das fühlt sich vom Layering her etwas unsauber an. Zum einen muss man im Interface über Text/Newlines reden, damit die Formlib das richtige Widget draus macht; das passt aber nicht recht dazu, dass im Domaincode darauf adaptiert, und sich dann im Gegenteil drauf verlassen wird, dieses Feld als Liste behandeln zu können. Andererseits wirds vmtl etwas umständlich, das zb mit einem IFormForOptions(IManualPublishOptions)
aufzuschreiben... aber dann sollte man die Mauschelei zumindest drankommentieren. ;)
Zum anderen brechen wir erstmal alles auf dicts runter, und stecken das dann wieder ins from_dict(), obwohl sowohl argparse als auch formlib einem eigentlich schon was getattr/setattr-basiertes in die Hand geben würden, was man direkt verwenden könnte; ich könnte mir vorstellen, dass das insgesamt mechanisch etwas einfacher wäre.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ja, da hast du Recht. Ich hab gesehen das wir ja sogar ein FileWidget innerhalb der Formlib haben. Damit kann ich unser CLI 1:1 in die Form übertragen 🥳
does nothing except making readability worse
Checklist
gif