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

Add CI check to validate package.resolved #643

Merged
merged 18 commits into from
Jan 22, 2025

Conversation

andrewdmontgomery
Copy link
Contributor

@andrewdmontgomery andrewdmontgomery commented Jan 20, 2025

Closes #642

Description

This adds a CI check to verify that the Package.resolved files are current. The pattern in this repo is to make the Makefile the source of truth for configuration settings (like the project scheme, in thie case), and then to have CI call the makefile task. So this PR follows that pattern.

Testing Steps

There is currently an out-of-date Package.resolved in the Demo project. We will fix that in this PR. For now, this branch does not contain the fix. So the new CI step should fail.

Once that test has failed, I will merge the updated Package.resolved.

Once this PR has merged into release/3.2.0 (with this fix), and once the back merge into trunk has merged, I will update the settings for trunk to require the new check.

  • CI should be 🟢

@wpmobilebot
Copy link

wpmobilebot commented Jan 20, 2025

Gravatar Prototype Build📲 You can test the changes from this Pull Request in Gravatar Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar Prototype Build Gravatar Prototype Build
Build Number2038
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-uikit.prototype-build
Commit2485922
App Center BuildGravatar SDK Demo - UIKit #563
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@andrewdmontgomery
Copy link
Contributor Author

The new "Package.resolved" step fails as expected:
image

And the logs show the modified Package.resolved being detected and failing the step:

[09:59:02]: Uncommitted changes detected: 
 M Demo/Gravatar-Demo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

@andrewdmontgomery
Copy link
Contributor Author

I reverted the ./Package.resolved in order to test against that location, too. It fails as expected:

[10:07:20]: Uncommitted changes detected: 
 M Demo/Gravatar-Demo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
 M Package.resolved

@andrewdmontgomery andrewdmontgomery requested review from a team and pinarol January 21, 2025 18:25
@AliSoftware
Copy link
Contributor

FYI we already have helpers for that in our a8c-ci-toolkit 🙃

https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/blob/trunk/bin/install_swiftpm_dependencies

(And even if it were to happen not to cover everything you wanted it'd probably be worth implementing any more of such checks in a8c-ci-toolkit directly so that all projects could benefit from them)

@andrewdmontgomery
Copy link
Contributor Author

FYI we already have helpers for that in our a8c-ci-toolkit 🙃

https://github.com/Automattic/a8c-ci-toolkit-buildkite-plugin/blob/trunk/bin/install_swiftpm_dependencies

(And even if it were to happen not to cover everything you wanted it'd probably be worth implementing any more of such checks in a8c-ci-toolkit directly so that all projects could benefit from them)

Thanks for this. I can move the logic into the pipeline, and then use this to update the Package.resolved. And I can move the git status --porcelain check into a command script.

But perhaps, as you said, this is an opportunity to update the toolkit. I could add a flag to this plugin (e.g. --strict-package-check) that would exit non-zero if the Package.resolved was not current. I have a draft PR for this.

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Great 🎉

@pinarol pinarol merged commit 26a9066 into release/3.2.0 Jan 22, 2025
14 checks passed
@pinarol pinarol deleted the andrewdmontgomery/add-package.resolved-CI-check branch January 22, 2025 07:04
@AliSoftware
Copy link
Contributor

AliSoftware commented Jan 22, 2025

I also forgot to mention1 that for the specific case of detecting if Package.resolved was uncommited we also have a Danger plugin for that 😛
Especially these checks you can call from your Dangerfile.

Don't hesitate yo ask @iangmaia for assistance or additional info on those too, as he's the one who worked on Dangermattic the most.

Footnotes

  1. Well to be truthful, it's more that I kinda confused the SPM-related features implemented in our Buildkite plugin vs the ones in our Danger plugin 😅

andrewdmontgomery added a commit that referenced this pull request Jan 22, 2025
This reverts commit 26a9066, reversing
changes made to 570cf1b.
@andrewdmontgomery
Copy link
Contributor Author

Ah thanks for finding the Danger plugin. This seems like a better first for what we need. I'll investigate, and likely replace this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants