-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add deploy to .org functionality #8
base: trunk
Are you sure you want to change the base?
Conversation
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.
Overall, looks good, but there seems to be a lot of duplication and little consistency for doing the same things across the 3 files.
.gitignore
Outdated
@@ -319,6 +319,7 @@ bh_unicode_properties.cache | |||
GitHub.sublime-settings | |||
|
|||
### VisualStudioCode ### | |||
.vscode |
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.
This seems to be in direct opposition to the lines below where there are exceptions for certain files inside. Plus, putting it here makes the whole thing harder to update ...
Could we find a different set of conditions over at https://www.toptal.com/developers/gitignore to achieve the same thing? The link to edit the Gitignore should be at the bottom of the file.
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.
I'm actually surprised we make these exceptions. I'd like to know more of what is the use case here.
I started using a vscode extension that colors the windows of each open project differently to help when working with multiple open windows. However, the extension adds the file and some settings to .vscode/settings.json that would be irrelevant to any other dev working on the same repo. I could see a potential in some cases, but we don't all work with vscode or have any work processes that revolve around vscode and a specific set of extensions or settings.
Since adding the extension, all the repos I work on want to add this file, so I've been adding the .vscode line so they don't get committed. Alternatively I could also remove the !.vscode/settings.json
line - it would achieve the same.
Maybe it's better to remove all of those exceptions?
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.
I've done a little more digging on this, and IMO, I think we should remove the exceptions in our .gitignore. I've been playing around with creating a VSCode extension (for viewing/validating dot org readme files) in my free time, and now looking at that project these are the kinds of files included in the folder, and seem to be needed if you're going to commit the extension code. In my extension I'm using launch.json
and extension.json
, specific for the extension's functionality. It's also using settings.json
, but only for the color customizations I mentioned above.
What do you think?
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.
I've gone ahead and removed the exceptions, but happy to add them back if you think they're needed.
- name: Checkout Repository | ||
uses: actions/checkout@v3 | ||
|
||
- name: Install Composer Dependencies |
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.
Before instlaling composer, I could add this:
- name: Validate composer.json and composer.lock
run: composer validate --strict
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.
I've added this.
.github/workflows/push-deploy.yml
Outdated
uses: actions/checkout@v3 | ||
|
||
- name: Install Composer Dependencies | ||
run: composer install --no-dev |
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.
What do you think of changing this to: composer install --no-dev --ignore-platform-reqs --prefer-dist --optimize-autoloader --no-progress
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.
Yep, updated.
|
||
# Files | ||
.* | ||
composer.lock |
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.
Should we also ignore composer.json
since we're ignoring package.json
?
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.
That was my initial thought, but the dot org plugin review team specifically asked it be left in. They explained it in this response: https://to51.wordpress.com/2023/03/24/pt-task-force-hibt-open-sourcing/#comment-35196
.github/workflows/push-deploy.yml
Outdated
|
||
- name: Install Node Modules and Build Assets | ||
run: | | ||
npm install |
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.
I think it's better to use npm ci
in this context rather than npm install
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.
Agreed. Updated.
@@ -0,0 +1,27 @@ | |||
name: (Dry Run) Deploy to WordPress.org |
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.
Do we need a separate file for this? Couldn't we add a variable or something to the other one? Maybe like have it dry-run when creating a pre-release tag on GitHub?
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.
As far as I can tell, the only difference is line 27 so it essentially doubles our maintenance efforts.
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.
Agreed. I did initially look for a way to pass the variable conditionally but couldn't find one. I'll take another look and see if there's a way.
I like the idea of maybe using a dry run on plugin release build, as one option. Also, tbh, I'm not confident the dry-run helps us much. The only difference is the final SVN push is blocked. If the workflow fails for some reason before that, then it won't get pushed anyways. I'll also dig into this a bit more to see if that's indeed true.
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.
So, it looks like the svn commit is indeed the last step (except for generate_zip if set). As such, if any of the steps are going to fail, it will happen well before this step, making the dry run somewhat unnecessary - especially if we keep the deploy action separate and intentional.
The other point to note, this will also fail if there's no svn repo for a plugin - eg, for a partner site where the plugin is a one-off and won't ever be deployed to .org - which is common with most of our standalone plugins in the repo, and ties in to my comment below, and therefore the generate_zip would never run.
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.
I've removed this workflow.
npm ci | ||
npm run build | ||
|
||
- name: Zip Folder |
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.
Could we use 10up's script to do this? They have a generate-zip
option which sounds like it could replace this entire thing. Maybe we can minify the whole thing to one single workflow that does different things based on whether the release tag is a beta, pre-release, or stuff like that...
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.
I considered this but ultimately decided not to. I'm hesitant to do this mostly because we're not normally deploying to .org, but we do create new releases for some plugins for partner sites - and hence I didn't want the two workflows tied together. Also, 51 devs and (some?) TAMs typically only create releases, ie no pre-releases, beta, etc, and changing that creates another opportunity for "accidental" deploys if it becomes too complex, and so I wanted the deploy to .org be intentional.
I'm fine with changing this approach and consolidating down to a single/or fewer workflows if you think that's the better approach.
Changes proposed in this Pull Request