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

Adds tooling for version incrementing #13977

Merged
merged 11 commits into from
Nov 27, 2019
Merged

Conversation

ChaosExAnima
Copy link
Contributor

Changes proposed in this Pull Request:

  • This updates the tooling system to allow automatic version incrementing and sanitization as part of the build step.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • p9dueE-14S-p2

Testing instructions:

  1. Run tools/build-release-branch.sh new
  2. Provide normally invalid version, such as 9-beta.
  3. When asked, update files.

Expected that version number in branch and in files is as expected.

Proposed changelog entry for your changes:

  • N/A

@ChaosExAnima ChaosExAnima added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. Build labels Nov 6, 2019
@ChaosExAnima ChaosExAnima requested a review from a team November 6, 2019 20:02
@ChaosExAnima ChaosExAnima self-assigned this Nov 6, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 6, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against dba0242

sed -r -i "s/\"version\": \".+\"/\"version\": \"${NPM_TARGET_VERSION}\"/" package.json

# Commit changed files.
git commit -m "update version" jetpack.php package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need any verification (e.g. git diff -- "Does this look good? Y to commit".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can totally do that, good idea!

Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

This script could be useful for updating the versions on master as well, which is done once per release cycle. What do you think about creating a separate update-version.sh or something that can be use independently from the release branch management (as well as within)?

@ChaosExAnima
Copy link
Contributor Author

What do you think about creating a separate update-version.sh or something that can be use independently from the release branch management (as well as within)?

Sure! That would definitely simplify logic as well. Would we want to move the git commit logic over as well?

@dereksmart
Copy link
Member

move the git commit logic over

Yeah! I'd make the commit optional/reviewable as per Kraft's suggestion to verify.

@ChaosExAnima ChaosExAnima added this to the 8.0 milestone Nov 7, 2019
fi

# Replace all file contents.
sed -r -i "s/Version: .+/Version: ${TARGET_VERSION}/" jetpack.php
Copy link
Member

Choose a reason for hiding this comment

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

I don't have access to -r on my machine:

sed: illegal option -- r
usage: sed script [-Ealn] [-i extension] [file ...]
       sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...]

I use macOS's default sed:

❯ which sed
/usr/bin/sed

I suspect I could get this fixed by installing a more recent version via Homebrew, but the same problem could happen to others in the future. Should we require a specific version of sed to run the script and bail early with an error inviting you to install if you don't have the right version? Or can we work around this somehow?

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 suspect that's because I'm using Ubuntu to develop this. Let me do some digging.

tools/version-update.sh Outdated Show resolved Hide resolved
tools/build-release-branch.sh Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 8, 2019
@ChaosExAnima ChaosExAnima added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 12, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This doesn't quite work just yet. I get the following errors when testing:

Would you like to update the version number in files to 333.4? [y/N]y
sed: 1: "jetpack.php": invalid command code j
sed: 1: "jetpack.php": invalid command code j
sed: 1: "package.json": extra characters at the end of p command

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 18, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Nov 18, 2019

I got it to work be updating the seds to:

sed -E -i '' "s/Version: .+/Version: ${TARGET_VERSION}/" jetpack.php
sed -E -i '' "s/'JETPACK__VERSION',( +)'(.+)'/'JETPACK__VERSION',\1'${TARGET_VERSION}'/" jetpack.php
sed -E -i '' "s/\"version\": \".+\"/\"version\": \"${NPM_TARGET_VERSION}\"/" package.json

It seems like sed on OS X didn't like the whitespace selector for me and -i needed the empty string?

@ChaosExAnima
Copy link
Contributor Author

@jeherve - Just updated with something that I tested to work with the macOS version of sed. Can you retest, please?

@ChaosExAnima ChaosExAnima added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 18, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Now it can edit the version numbers for me.

I am not sure if I did this wrong, but when I provide a version like 555.23, it updates to that number directly. When I provide a version like 555.23-beta, it also updates the version numbers directly but the branch that gets created also uses the -beta suffix, branch-555.23-beta. Ideally it would get removed, don't you think?

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 19, 2019
@ChaosExAnima ChaosExAnima removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 19, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This is working well! I only have one final request: could we also update JETPACK__VERSION when we update version numbers via the script?

@ChaosExAnima
Copy link
Contributor Author

@jeherve - Okay, resolved. Apparently macOS sed does not like \s, but I tested again and it looks good to me :)

@ChaosExAnima ChaosExAnima added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 26, 2019
@jeherve
Copy link
Member

jeherve commented Nov 27, 2019

This seems to work well for me now. It is only failing because of phpcs-changed:

FILE: jetpack.php
-----------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 0 WARNINGS AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------
 18 | ERROR | Expected 1 space after comma in argument list; 13 found
-----------------------------------------------------------------------------------------------

This should fix that: #14134

@jeherve jeherve added [Status] Blocked / Hold and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 27, 2019
@jeherve jeherve force-pushed the update/version-incrementer branch from 8cf8feb to dba0242 Compare November 27, 2019 17:02
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Blocked / Hold [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 27, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Rebased to include the fix from the other PR. This should be good to merge now!

@ChaosExAnima ChaosExAnima merged commit 154db22 into master Nov 27, 2019
@ChaosExAnima ChaosExAnima deleted the update/version-incrementer branch November 27, 2019 17:30
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants