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

Run start code freeze on tumblr-metal agent #1649

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Sep 4, 2024

What it say in the title. Follows up to #1646 (comment)

I will use my admin privileges to merge on trunk in order to test the process. The code is all an adaptation from other projects that have already used it in production for a while.

I'd be thankful for a posthumous review and will address any feedback coming that way, @Automattic/apps-infrastructure . Thanks!

@dangermattic
Copy link
Collaborator

dangermattic commented Sep 4, 2024

1 Warning
⚠️ This PR is assigned to the milestone 4.53. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@mokagio mokagio added this to the 4.53 milestone Sep 4, 2024
@mokagio mokagio added the tooling Related to anything that supports the building & maintaining of the project. label Sep 4, 2024
@wpmobilebot
Copy link
Collaborator

Simplenote Prototype Build📲 You can test the changes from this Pull Request in Simplenote Prototype Build by scanning the QR code below to install the corresponding build.
App NameSimplenote Prototype Build Simplenote Prototype Build
Build Numberpr1649-16d6b4c-0191bb79-5eb8-491c-8a20-1179af7ebbdd
Version4.52
Bundle IDcom.codality.NotationalFlow.Alpha
Commit16d6b4c
App Center BuildSimplenote - Installable Builds #356
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio
Copy link
Contributor Author

mokagio commented Sep 4, 2024

Merging without waiting for the UI tests because this is not an app source PR.

@mokagio mokagio merged commit 2f637d6 into trunk Sep 4, 2024
9 of 12 checks passed
@mokagio mokagio deleted the mokagio/run-start-code-freeze-on-tumblr-metal branch September 4, 2024 05:24
Comment on lines 10 to 11
# The first client to implement releases in CI was Android so the automation works in that queue.
# We might want to move it to a leaner one in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this comment be removed now that it has been moved to a different agent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this!

27f7fe6

.buildkite/commands/configure-git-for-release-management.sh

echo '--- :ruby: Set up Ruby Tools'
install_gems
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This script could have come in handy in the future, to be honest. But in looking at it I realized a minor issue: it installs the gems before switching branch. This is unlikely to cause issues, but it might in some rare occasions. Given this limitation and the fact that it's used only once at the moment (YAGNI) I decided to remove it and delegate my future self to DRY in a better way if required.


# Buildkite is currently using the HTTPS URL to checkout.
# We need to override it to be able to use the deploy key.
git remote set-url origin [email protected]:Automattic/simplenote-ios.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous version is how WordPress does it, https://github.com/wordpress-mobile/WordPress-iOS/blob/b4de5f928cd784cb85144ca469b27910666d8d30/.buildkite/commands/configure-git-for-release-management.sh, this how WooCommerce does it, https://github.com/woocommerce/woocommerce-ios/blob/c2f38422ddb12b891afbab9962869affa7722af2/.buildkite/commands/configure-git-for-release-management.sh

Given Woo is a later iteration, I decided to adopt it.

I also secretly hope it will help with the Git access that prompted this PR when we'll get to run the complete code freeze, but I don't see how.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also secretly hope it will help with the Git access that prompted this PR when we'll get to run the complete code freeze, but I don't see how.

As expected, this wasn't enough.

But, one only had to read the error in CI more closely to understand what the problem was:

ERROR: The key you are authenticating with has been marked as read only.

Unsurprisingly, the deploy key in this repo was read-only!

Once replaced with a read-write one, everything worked. :okay:

Copy link
Contributor

Choose a reason for hiding this comment

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

@mokagio But wasn't the whole point to keep the deploy key set in GitHub as read-only for security purposes? And use use-bot-for-git.sh on a trusted agent for the push action?

Copy link
Contributor

Choose a reason for hiding this comment

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

True; the deploy key of the repo should be used only for cloning the repo in non-release pipelines (therefore being read-only). In release pipelines (which at some point should run on a trusted agent) such as the one calling start_code_freeze, we wouldn't use the repo deploy key at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iangmaia @AliSoftware start_code_freeze runs on the trusted agent:

agents:
queue: tumblr-metal

The problem is with complete_code_freeze, which needs to run on macOS because it accesses genstrings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Related to anything that supports the building & maintaining of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants