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

Deprecate Deliverfile in favor of Fastfile #1515

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

iangmaia
Copy link
Contributor

@iangmaia iangmaia commented Feb 6, 2023

Fix

Similarly to what was done for WPiOS on wordpress-mobile/WordPress-iOS#16805, this PR removes the Deliverfile moving everything to a new lane in the Fastfile. For that I also moved out the privacy_url and the copyright to separate files in the metadata folder.
I assumed the existing lane upload_screenshots could then be removed in favour of the new lane, update_metadata_on_app_store_connect, which will be responsible for both the metadata upload as well as the screenshots upload.

Additionally, the need for an app_version set in the Deliverfile has been deprecated and its usage is gonna soon be removed altogether from release-toolkit, so this PR already cleans this up.

Test

Testing the new lane is similar to wordpress-mobile/WordPress-iOS#16805 (and is particularly difficult for me to do completely, so I'd also appreciate some help):

  1. Checkout this branch
  2. Run bundle exec fastlane update_metadata_on_app_store_connect
  3. Verify the Preview.html contains valid data. In particular, that it's not attempting to upload screenshots
  4. Cancel the upload 🙏 Does the Preview on path './fastlane/Preview.html' look okay for you? (y/n) n

Also good to test the use of the parameter with_screenshots:true to see that it will attempt to load the screenshots.

Review

Since one lane was removed and another one was added, I imagine the release process needs to be amended as well.
Please review carefully the parameters used in the new lane update_metadata_on_app_store_connect, making sure it covers the current use cases.

I have also noticed that this project uses an older version of the release-toolkit, so perhaps this upgrade needs to happen before this PR lands.

Release

These changes do not require release notes.

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

Similar to woocommerce/woocommerce-ios#8831 (review), we need to add skip_deliver: true to the call site which will try to bump the version from the Deliverfile now that that file is gone (see wordpress-mobile/WordPress-iOS@6b88866)

@AliSoftware
Copy link
Contributor

Since one lane was removed and another one was added, I imagine the release process needs to be amended as well.
Please review carefully the parameters used in the new lane update_metadata_on_app_store_connect, making sure it covers the current use cases.

I took a look and indeed it seems that getting rid of the old lane makes sense as the new lane also takes care of what the old lane covered. And as it happens, I checked in our release scenario for Simplenote-iOS (private repo) and couldn't find a reference to that old upload_screenshots lane being mentioned (I think that lane was only used adhoc/when needed only, as opposed to being listed as a step that had to be run on every release… hence why it's not in the scenario)

I have also noticed that this project uses an older version of the release-toolkit, so perhaps this upgrade needs to happen before this PR lands.

Good point and nice to have noticed that 👍 .

But that's all right. Simplenote is currently in maintenance mode, so that's not surprising that it's on an older version of the toolkit, and we don't plan to update it that soon (we don't know when it's going to go out of maintenance mode and we'll have time to work back on it), so we kinda already know that there will be more work to be done on other places in its Fastfile (to fix other brealing changes from 6.x) in future unrelated PRs before being able to update to the latest toolkit anyway 🙃

@iangmaia
Copy link
Contributor Author

I took a look and indeed it seems that getting rid of the old lane makes sense as the new lane also takes care of what the old lane covered. And as it happens, I checked in our release scenario for Simplenote-iOS (private repo) and couldn't find a reference to that old upload_screenshots lane being mentioned (I think that lane was only used adhoc/when needed only, as opposed to being listed as a step that had to be run on every release… hence why it's not in the scenario)

Ah, it makes sense then for it to be removed and the functionality done in that single lane. Thanks for checking! 👍

But that's all right. Simplenote is currently in maintenance mode, so that's not surprising that it's on an older version of the toolkit, and we don't plan to update it that soon (we don't know when it's going to go out of maintenance mode and we'll have time to work back on it), so we kinda already know that there will be more work to be done on other places in its Fastfile (to fix other brealing changes from 6.x) in future unrelated PRs before being able to update to the latest toolkit anyway 🙃

Got it 👍 Yeah, indeed it will need more work to catch up with the latest changes.

@iangmaia iangmaia marked this pull request as ready for review February 10, 2023 18:26
@AliSoftware AliSoftware merged commit b1061e8 into Automattic:trunk Feb 10, 2023
@iangmaia iangmaia deleted the cleanup/deprecate-deliver-file branch February 24, 2023 12:38
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.

2 participants