Conversation
With the --delete-excluded flag, it would be removed from SVN if it doesn't exist in Git
| type: string | ||
| required: true | ||
|
|
||
| TEST_MODE: |
There was a problem hiding this comment.
note
I would rename this to DRY_RUN.
| --exclude='node_modules' \ | ||
| --exclude='.github' \ | ||
| --exclude='.ddev' \ | ||
| --exclude-from='.distignore' \ |
There was a problem hiding this comment.
question:
If we are already respecting .distignore do we even need to exclude other files on top of it?
And again: If we are intending to sync production artifacts from a build branch anyway, should we even dabble with bespoke exclude lists in the first place?
The expectation should be that everything that is present in the artifact should also be synced to SVN - since the build process already applied the .distignore.
Or are you explicitly aiming to support "simple" plugin publishing workflows directly off a dev branch? Is this something we need?
There was a problem hiding this comment.
I'm assuming we don't know when/how this reusable workflow will be used. Not all projects use Build and Distribute yet, so this workflow should work on dev branches, too.
The exclusions provide sensible defaults (.git, node_modules, auth.json) that are universally unwanted on wordpress.org. Projects can extend this via .distignore for their specific needs (note: the file is optional, so branches after build-and-distribute work fine).
What's the intended scope? I'd keep it as it is, but I'm open to removing exclusions if you are sure they are pointless.
There was a problem hiding this comment.
To me this is less about build&distribute in particular and more about the question if this workflow should be about publishing or about postprocessing and publishing
My gut reaction what that "whatever lands here is intentionally in that state": If the release artifact wants to include the composer.json, why is our workflow gatekeeping that?
That said, I could be sacrificing security for ontologic purity. A pragmatic safety net against erroneously pushing sensitive stuff is hard to really argue against.
suggestion:
Perhaps though, a .distignore is really all we need?
Test if the file exists (instead of creating an empty one).
Remove it after applying.
I would like others to weigh in on this as well. For me it's not a blocker, but I do see room for improvements.
There was a problem hiding this comment.
After thinking a bit, I agree that excluding files like composer.json is probably too much. How about the middle ground like this:
- exclude from .distignore if it exists;
- exclude
.envandauth.jsonbecause of security - remove the rest of the exclusions
?
| PLUGIN_VERSION: | ||
| description: "Plugin version to publish (MAJOR.MINOR.PATCH)" | ||
| type: string | ||
| required: true |
There was a problem hiding this comment.
suggestion:
We could remove this input or make it optional by extracting the plugin version from the main plugin file:
sed -n 's/.*Version:[[:space:]]*\([^[:space:]]*\).*/\1/p'
Given that updating the version number in that file is a critical part of the release process anyway, we might not want to leave room for error here.
There was a problem hiding this comment.
I was considering this, but decided against. I did it to prioritize explicitness and minimize any magic that could go wrong. For example, one might forget to update version in the main plugin file. In combination with tag overriding, this may release a new version with the old version number. Admittedly, I forgot once to update version in the main plugin file myself, so I know who this limitation is for :)
Another reason is that I left it to a calling workflow to decide whether to derive any inputs automatically. If some projects need it, and it is safe enough for them, they surely can implement this in the action, reducing the number of inputs for users.
Maybe it would be better to make sure that the entered version, the version in the main plugin file, and Stable tag in readme.txt are the same? This would drastically reduce the space for version-related incidents.
Nevertheless, I'm happy to revisit this if the team feels strongly about version auto-extraction. Just wanted to explain my reasoning first.
There was a problem hiding this comment.
Thank you for sharing your thoughts. I can agree that adding the sed call is trivial to add for the calling workflows so it is easily set up per-project while keeping the magic out of the workflow itsel. Perhaps we can document the auto-extraction as a usage example.
| if [[ ! "$PLUGIN_VERSION" =~ ^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then | ||
| echo "❌ WordPress.org expects version formatted as MAJOR.MINOR.PATCH" | ||
| echo " Three groups of numbers separated by dots" | ||
| echo " Each group: either 0 or digits not starting with 0" | ||
| echo " Received: $PLUGIN_VERSION" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
question:
Just to clarify: This prevents us from
- Syncing only to trunk (a relatively common pattern to share dev previews)
- Updating an existing version/tag for whatever reason. (Sometime used to fix non-critical slips during release). If we ever find ourselves in that situation, we are out of options.
...am I seeing this correctly?
In my opinion, neither of them are real deal-breakers.
- Syncing trunk is not something we've done in the past - admittedly: mostly due to the lack of a frictionless workflow - and I cannot say I really missed this capability. However, it might open up some use-cases to have a clean distribution channel for the upcoming release.
- Updating an existing version is mostly required because of a release process that leaves too much room for errors - which is exactly what I hope will improve with this workflow. So adding loopholes because of past anxiety is not a smart move.
That said: This check begs the question why we have to enforce it so hard - to the point of making the workflow fail.
In essence, this tag handling and this particular check means the workflow is hardcoded to handle new plugin releases only. Is this aligned with everyone?
There was a problem hiding this comment.
Is this aligned with everyone?
Well, there were no detailed requirements, so I took the liberty of making a decision and listening to feedback. So here we are :)
Syncing only to trunk (a relatively common pattern to share dev previews)
If we need this, I have no problem making it possible. But to me, adding such a feature creates more potential problems than abilities. For example:
- How to proceed if we only updated trunk? Do we need a separate workflow or the ability to resume this one?
- How to handle updating Stable tag in readme.txt?
- There are simply more combinations and failure points in this case.
This gives me a good idea, valid in any case: to check that the given version is reflected in readme.txt and in the main plugin file.
Updating an existing version/tag for whatever reason.
I can see benefits from updating existing tags. But overriding tags looked rather dangerous. It is much more likely that somebody accidentally overrides the existing version than that we need to do it intentionally. I suggest a middle ground here: optional input for allowing overriding versions. So if anybody needs it, they'll have to enable it explicitly.
This check begs the question why we have to enforce it so hard - to the point of making the workflow fail.
Overall, my idea was to fail and exit in case of any issues rather than letting it proceed with warnings and potentially get a serious problem. Since this is a public release, the responsibility is high, so failing early seems to be the best strategy.
What do you think about a separate input to allow overriding tags? And about validating Stable tag?
There was a problem hiding this comment.
Good feedback! Much appreciated.
I agree it is hard to please everyone and have one workflow that does everything. I think we extracted three key use-cases:
- Create new release
- Amend existing release
- Sync to trunk
One way forward would be to add a workflow_dispatch.inputs.type: choice so we can select the concrete action. Based on the input, we could apply individual checks and error cases - and then set up the SVN working dir accordingly. Luckily, SVN only really cares about the directory structure, so an added input for these use-cases would be almost validation-only - and the stable tag validation is definitely a highly valuable check to include here.
Let's see what others think.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
There is no reusable workflow for wordpress.org release
Addresses SPP-91.
What is the new behavior (if this is a feature change)?
Implemented a reusable workflow for publishing a plugin.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
Features
.git,.github,.ddev,node_modules), build configs (composer.json,package.json, etc.), and sensitive files (auth.json,.env).distignorefile for custom exclusions beyond the default filter listOut of scope
Note: proper documentation is missing so far. I'm going to add it as soon as the general concept is approved.
Example calling action