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

[WIP] Platform Release: Improve patch and dev version support #188

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

brodycj
Copy link

@brodycj brodycj commented Aug 2, 2018

Platforms affected

All

What does this PR do?

TBD should be done more completely, in a separate PR:

  • fix usage messages

TODO: these items should be done in a separate PR:

  • prepare iOS CDVAvailability.h after updating JS in git (more sensical order)
  • do not update iOS CDVAvailability.h if version number ends with non-numerical character, needed to avoid build error in case of marking -dev version

Proposed items:

  • workaround for local grunt issue in cordova-js from PR WORKAROUND SOLUTION for local grunt issue in cordova-js #174 (hope to look into a more "real" solution, someday, will probably comment more in WORKAROUND SOLUTION for local grunt issue in cordova-js #174)
  • Updates to improve support for patch releases (see rationale in PR Improve patch release support #176)
    • prepare-platform-release-branch add options:
      • [-b <platform branch name>]
      • [-js <cordova-js branch or tag name>]
    • copy-js add option:
      • [-js <cordova-js branch or tag name>]
  • Add --tag-only option to tag-release command (shows the git push commands that can be done manually)
  • EXTRA GIT WORKAROUND solution to do git checkout -- package.json, needed for strange cases with cordova-osx (4.0.2-dev) & cordova-windows
  • show slightly modified commit message in case version is updated in package.json
  • add --pre option to prepare-platform-release-branch to allow specific prefix such as JIRA or GH number in the commit messages
  • remove some extra info from commit messages
  • other minor fixes

This proposal supersedes and resolves #174 (GH-174).

What testing has been done on this change?

GH-174 was successfully used for a number of patch releases including:

(updated usage as needed for the patch releases is shown in most of these PRs)

Additional changes to add the --pre option & better deal with -dev version are demonstrated working in:

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

Christopher J. Brody added 12 commits August 1, 2018 19:04
- prepare-platform-release-branch add options:
  - [-b <platform branch name>]
  - [--js <cordova-js branch or tag name>]

- copy-js add option:
  - [--js <cordova-js branch or tag name>]
needed for cordova-osx (4.0.2-dev) & cordova-windows

FUTURE TBD can we think of a better fix?
according to recent changes:
- usage with non-master release branch
- tag without automatic push
needed in case of commit prefix such as --pre='CB-12345 '
@janpio janpio changed the title Improve patch and dev version support Platform Release: Improve patch and dev version support Aug 2, 2018
src/gitutil.js Show resolved Hide resolved
src/platform-release.js Outdated Show resolved Hide resolved
@janpio
Copy link
Member

janpio commented Aug 2, 2018

workaround for local grunt issue in cordova-js from PR #174

What does this mean? Issue in a PR?

This proposal supersedes and resolves #174 (GH-174).

#174 is already closed though!?

... needed for strange cases with cordova-osx (4.0.2-dev) & cordova-windows

What strange cases?

GH-174 was successfully used for a number of patch releases including:
...

What does this mean in the context of this PR? I don't get the connection.

@janpio
Copy link
Member

janpio commented Aug 2, 2018

In general: I think this should not be 1 PR, but several that each do less things and have a clear problem description in the PR description (or link to an issue somewhere). Right now this is too much to understand and assign which problem is solved by which line of change.

@brodycj
Copy link
Author

brodycj commented Aug 2, 2018

Thanks @janpio for reviewing. I have been keeping this branch to solve some mostly basic problems with making branch releases & dev versions. Unfortunately I haven't had so much time to go through and document the exact problems in better detail.

I would like to finish with what I think are some urgent tasks remaining then can go through, document in more detail, and raise in separate PRs for review.

Does that sound OK?

@brodycj brodycj changed the title Platform Release: Improve patch and dev version support [WIP] Platform Release: Improve patch and dev version support Aug 2, 2018
@janpio
Copy link
Member

janpio commented Aug 2, 2018

Yep, not in a hurry - just wanted to make sure that all open questions to the changes being made are answered and we won't be mad in a year or so about that massive commit without all the necessary context ;)

@janpio
Copy link
Member

janpio commented May 21, 2019

Hey @brodybits, after going through the release process documentation again to try to make sense of it (and possibly unify it for all repos), I think most of the actual changes on coho you suggest here make sense and might be worth splitting into individual PRs to be merged as soon as possible.

(I am not sure about the package.json and grunt stuff)

Do you possibly have the time to fix the tiny merge conflict here and then split this into individual PRs?

Chris Brody and others added 6 commits May 21, 2019 12:45
as suggested by @janpio

Co-Authored-By: Jan Piotrowski <piotrowski+github@gmail.com>
(but keep vertical spacing)

- test for version with all digits before updating CDVAvailability.h - a411022
- move the code to Prepare CDVAvailability.h - 7b54e27.

in order to avoid merge conflict with master
(in place of XXX TBD ??? comments that were added before)
@@ -228,13 +228,15 @@ exports.prepareReleaseBranchCommand = function * () {
yield executil.execHelper(executil.ARGS('git checkout -b ' + branchName));
}

print(repo.repoName + ': Update JS snapshot version for VERSION "' + version + '" on branch "' + branchName + '".');
Copy link
Author

Choose a reason for hiding this comment

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

Both changes from this commit (aefb169) were clobbered by a411022 and will likely go away.

yield updateJsSnapshot(repo, version, true);

print(repo.repoName + ': Setting VERSION to "' + version + '" on branch "' + branchName + '".');
yield versionutil.updateRepoVersion(repo, version);

if (platform === 'ios') {
// Updates version in CDVAvailability.h file
print(repo.repoName + ': Update CDVAvailability.h for VERSION to "' + version + '" on branch "' + branchName + '".');
Copy link
Author

Choose a reason for hiding this comment

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

Both changes from this commit (aefb169) were clobbered by a411022 and will likely go away.

@brodycj
Copy link
Author

brodycj commented May 21, 2019

I just merged in the changes from master, updated some XXX TBD comments, added some TODO comments, and did eslint --fix. I reverted some changes that I made for iOS to avoid a merge conflict for now.

I also rebased the changes with 7b54e27 and aefb169 removed: master...brodybits:pr-188-rebased

Unfortunately I cannot promise when I will get a chance to split the changes as I am falling behind on some other commitments.

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