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

Update deploy.yml #543

Merged
merged 3 commits into from
Mar 24, 2023
Merged

Update deploy.yml #543

merged 3 commits into from
Mar 24, 2023

Conversation

jeffpaul
Copy link
Member

What?

Updates the current deploy.yml file by swapping in https://github.com/10up/action-wordpress-plugin-deploy for the steps to deploy to SVN.

Why?

Deploys to SVN are disabled as they're currently failing, see:

# Disable deployments while they are failing for unknown reason.
.

How?

Replaces the previous Authenticate with WordPress.org SVN and Deploy to WordPress.org SVN steps with WordPress Plugin Deploy and Upload release asset steps utilizing the https://github.com/10up/action-wordpress-plugin-deploy GitHub Action.

Note that this also ONLY deploys on published releases and no longer on ANY merge to master which means that only newly released versions will be deployed to SVN and any individual merges to master on GitHub will no longer be deployed to trunk on SVN.

Testing Instructions

This is a tough one, pretty much need to YOLO into a published release and 🤞🏼 that the Action processes correctly (e.g., that SVN_USERNAME, SVN_PASSWORD, and GITHUB_TOKEN are accurate).

Screenshots or screencast

n/a

Changelog Entry

Changed - Deploy workflow uses 10up/action-wordpress-plugin-deploy instead of a manual connection to SVN.

@jeffpaul jeffpaul self-assigned this Mar 20, 2023
@jeffpaul jeffpaul requested review from kasparsd, dd32 and iandunn March 20, 2023 18:41
@jeffpaul
Copy link
Member Author

Open to whether we try this with 0.8.0 or figure out post-release.

This was referenced Mar 20, 2023
Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

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

I don't have a strong opinion, but this sounds good to me 👍🏻

Copy link
Member

@dd32 dd32 left a comment

Choose a reason for hiding this comment

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

I have no strong opinion here. I can confirm all the secret constants are correct.

However, I'm 99% certain this won't work as-is, see suggestions:

  • npm run build needs to happen after npm install to actually build the plugin (npm run deploy did this previously)
  • BUILD_DIR: dist needs to be added to the env

I can't test this, as npm run build doesn't work on my environments,

.github/workflows/deploy.yml Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

This looks good!

The build process is being tested as part of this step in the other CI file:

- name: Build
run: npm run build

and it is running correctly:

https://github.com/WordPress/two-factor/actions/runs/4471733733/jobs/7856996785?pr=543#step:12:17

As long as the deploy action picks up the files at dist/* (after implementing the suggestion from @dd32), it should be all good!

.github/workflows/deploy.yml Show resolved Hide resolved
@kasparsd
Copy link
Collaborator

I can't test this, as npm run build doesn't work on my environments

@dd32 Do you know why that is? Should we create an issue to fix that? Technically all of the build logic should be handled by Node with no external system dependencies. Would moving everything to wp-scripts solve the issue?

@dd32
Copy link
Member

dd32 commented Mar 23, 2023

I can't test this, as npm run build doesn't work on my environments

dd32 Do you know why that is? Should we create an issue to fix

And suddenly now when I went to copy the error, everything works. Magic. (I haven't tested this PR, but visual review looks like it'll work to me)

Sorry for the false alarm.

@jeffpaul jeffpaul added this to the 0.8.0 milestone Mar 23, 2023
@jeffpaul
Copy link
Member Author

@kasparsd if you're 👍🏼 on this, feel free to merge and then proceed with the steps on #533 to complete the 0.8.0 release (happy to handle those myself if you're good on this PR to ensure we're not blocked on the release deploy to SVN).

Noting here that ideally we'd sneak #540 into 0.8.0 but I do not view that as a blocker.

@kasparsd kasparsd merged commit 48ce9e4 into master Mar 24, 2023
@kasparsd kasparsd deleted the update/deploy-action branch March 24, 2023 13:33
dd32 added a commit to dd32/two-factor that referenced this pull request Aug 22, 2024
dd32 added a commit that referenced this pull request Aug 22, 2024
@kasparsd kasparsd mentioned this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants