[DRAFT] wordpress.org release (SPP-91)#228
Conversation
With the --delete-excluded flag, it would be removed from SVN if it doesn't exist in Git
| --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
?
There was a problem hiding this comment.
I only left excluding from .distignore, .svn, and sensitive files like auth.json, .npmrc, and .env.
If you really want to follow the idea of 'whatever lands here, should be published', I could also remove .distignore, but I'd at least keep these sensitive files excluded. For a slightly lower purity, we are getting a guard against a realistic incident path.
| 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.
There was a problem hiding this comment.
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
I like this safety net, as it helps with release confidence when the person doing a release usually is out of office or catch careless errors due to urgent hot-fix releases.
An alternative would be to overwrite those two places with the entered version
There was a problem hiding this comment.
I added validation for versions from the input, main plugin file, and readme.txt. If they mismatch, the execution stops with an error listing all mismatches.
The idea behind this is that fixing the version and restarting the job is easy and safe. Editing an existing release or publishing a new one if versions are mixed up is complicated and painful.
I can add an example for extracting the version from the main plugin file to the documentation, so that anybody can use it in their action, if they want to.
@stracker-phil @Biont Are you both ok with this?
| 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.
There was a problem hiding this comment.
Based on recent experiences and discussions, I would like to put a bit more weight on Sync to trunk. This gives us a last line of defense before publishing - as well as a way to verify and test the artifact to be published. With the added benefit that we can already download it off wordpress.org so we can even run automation against it.
There was a problem hiding this comment.
I split it into two jobs to achieve this, or something very similar. The first job does validation, preparation, and pushes changes to trunk.
The second one creates a tag, using a specific environment. We can use environments to pause the process between these jobs, let QA test what is pushed to the trunk, and even configure who can allow creating the tag.
If the project doesn't need approval in between steps, it is enough to just not create the environment in the repository settings. If there is no referenced environment, it will be created without any restricting rules. So it should look exactly as if there were only one job. The only downside is that an empty environment would be created.
The first one does validation and pushes changes to the trunk. The second one creates a new tag from the trunk at wordpress.org This gives the ability to set up environment guard and test changes already pushed to trunk before creating a new tag.
There is no way we can get a conflict in our workflow
Biont
left a comment
There was a problem hiding this comment.
The two-job structure with an environment gate is a genuinely good solution to the staged release problem, and the version cross-validation across PLUGIN_VERSION, the plugin file, and readme.txt is a solid safety net. The overall approach is sound and most of the earlier review feedback has been addressed well.
One concern worth discussing before this lands: environment: wordpress-org-release introduces a dependency on a project-side GitHub environment, and this is a concept with no precedent in this repository's existing workflows.
On first run, every calling repository will have that environment auto-created regardless of whether they intend to use an approval gate, which quietly pollutes project settings and nudges teams toward a naming convention that has never been established or discussed. Making the environment name an optional input — defaulting to 'wordpress-org-release' — would remove the naming coupling and let projects use their own conventions.
But even that is a band-aid on a broader question: should this workflow reach into project-level deployment configuration at all before we've had a cross-team conversation about how environments should be used across repositories?
A second gap is the trunk-only flow. The current design only supports a full release: sync trunk, then create a tag. DRY_RUN: true produces an artifact rather than a real SVN commit, so there is no way in live mode to push to SVN trunk and stop cleanly without a tag following.
The only current escape is cancelling a pending approval, which leaves trunk committed and the run in a failed state. I raised trunk-only as a meaningful use case on April 22nd — it enables testing the published artifact before tagging — and a TRUNK_ONLY input (or my earlier OPERATION: choice that cleanly unifies new-release, trunk-only, and amend-tag) would close this without much added complexity.
I would also like to stress that the current design includes a failure mode that we must address. It falls out of the decision to leverage deployment environments - combined with Github's default behaviour:
- Developers set up the workflow not reading docs carefully, not expecting any harm to be done
- Thus, they do not setup and configure
'wordpress-org-release'on the project - Github therefore auto-creates the environment on first run
- Workflow sync trunk and proceeds to the tag creation step
- The newly created env now contains no checks & approvals whatsoever
- Because of that, it will auto-approve and create a new release without approval
Again: I support the idea of environments in principle. But I would really like to see a harmless test-run in an isolated project before we make this a quasi-standard via our shared reusable workflows.
Finally, the amend-tag case remains unimplemented: the "Verify version doesn't exist in SVN" check hard-exits with no bypass.
Before merging it would be worth an explicit decision: is this workflow intentionally scoped to new releases only? If so, that should be documented clearly so teams know in advance what to do when they need one of the other flows.
This reverts commit 7372049.
…ease' into feature/SPP-91-wordpress-org-release
…ease' into feature/SPP-91-wordpress-org-release
|
@Biont As we agreed in the call, I replaced the environment usage with the I also added documentation with three example calling actions: minimal, invoked on tag creation, and using environments for manual approval. |
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
auth.json,.npmrc,.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