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

Publish GitHub release from master branch #7136

Merged
merged 5 commits into from
Sep 11, 2019

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Sep 10, 2019

This ensures that changes made on develop since branching for the release are not included. It also ensures that the final release sourcemaps line-up correctly (they were always build on master)`.

@Gudahtt Gudahtt requested review from whymarrh and kumavis September 10, 2019 00:52
@metamaskbot
Copy link
Collaborator

Builds ready [5f8d52b]: chrome, firefox, edge, opera

Copy link
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

having trouble understanding the flow

seems like job-publish-release and create_github_release should be two parts of the same flow

@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 10, 2019

Yes, they probably should be. Good point - that might be worth changing now, to ensure they aren't separated again.

kumavis
kumavis previously approved these changes Sep 10, 2019
Copy link
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

great! still want to get an OK from @whymarrh first

as long as they are together, i dont mind as much what branch they build from

@metamaskbot
Copy link
Collaborator

Builds ready [c80f1cf]: chrome, firefox, edge, opera

@kumavis
Copy link
Member

kumavis commented Sep 10, 2019

also relevant: im of the opinion we should get rid of the master branch (or rename develop to master). i think its all pain no gain. automated releases now correctly set a tag, so most of the reason to have master is covered.

@kumavis
Copy link
Member

kumavis commented Sep 10, 2019

so maybe for this PR, now that they are a single task, we can have it run on develop or the feature branch. Alternatively, if its hard to architect with circleCI, we could try github actions. I think that lets us hook into richer actions like PR merge.

@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 10, 2019

The problem with targeting the release branch at develop and building upon merge is that we'd then include anything merged into develop since the release branch was created.

I'm not sure how we could trigger either CircleCI or GitHub Actions to build on the last commit of the branch being merged, rather than the merge commit itself. If we could find a way to do that, that'd be great.

Instead we might need to come up with an alternate means of triggering the release - some event other than the merging of the release branch. What I was considering using is the git tag itself - we could build upon that being added, and even have CircleCI/Actions merge the release branch for us. So then the workflow would be to add the tag when the release was ready, and the rest would be automated. That's a bit of a weird workflow though - there are alternatives we could use that involve reviews/comments being the trigger instead, that we'd need either a GitHub App/bot or GitHub Actions to facilitate.

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

We'll need to do something with the grep pattern we're using in the script:

if grep --quiet '^Version v' <<< "$current_commit_msg"

This ensures that changes made on `develop` since branching for the
release are not included. It also ensures that the final release
sourcemaps line-up correctly (they were always build on master)`.
The jobs `job-publish-release` and `create_github_release` both handle
different parts of publishing a release. They have been consolidated
into a single `job-publish-release` job.
The release script was originally written to be run on `develop`, so it
expected the current commit to be a result of `Squash & Merge`. Now
that it's run on `master`, it will generally be run against a merge
commit.

The version is now extracted from the commit message using a regular
expression that should work on all version of Bash v3+, and should be
tolerant of both merge commits and `Squash & Merge` commits.
`master` is now targetted by the release PR instead of `develop`, as
the release has to be created from the master branch.

The update to `develop` is handled after the release by a PR from
`master` to `develop`, which is created automatically after the
release.
@Gudahtt Gudahtt force-pushed the publish-master-github-release branch from ec99096 to c8b0c37 Compare September 10, 2019 19:23
@metamaskbot
Copy link
Collaborator

Builds ready [c8b0c37]: chrome, firefox, edge, opera

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Collaborator

Builds ready [8ecd7a8]: chrome, firefox, edge, opera

@Gudahtt Gudahtt merged commit c9fffaf into develop Sep 11, 2019
Gudahtt added a commit that referenced this pull request Sep 11, 2019
* Publish GitHub release from master branch

This ensures that changes made on `develop` since branching for the
release are not included. It also ensures that the final release
sourcemaps line-up correctly (they were always build on master)`.

* Consolidate publish jobs

The jobs `job-publish-release` and `create_github_release` both handle
different parts of publishing a release. They have been consolidated
into a single `job-publish-release` job.

* Update release script to expect a merge commit

The release script was originally written to be run on `develop`, so it
expected the current commit to be a result of `Squash & Merge`. Now
that it's run on `master`, it will generally be run against a merge
commit.

The version is now extracted from the commit message using a regular
expression that should work on all version of Bash v3+, and should be
tolerant of both merge commits and `Squash & Merge` commits.

* Target `master` with release PR

`master` is now targeted by the release PR instead of `develop`, as
the release has to be created from the master branch.

The update to `develop` is handled after the release by a PR from
`master` to `develop`, which is created automatically after the
release.
@whymarrh whymarrh deleted the publish-master-github-release branch September 11, 2019 16:40
Gudahtt added a commit that referenced this pull request Sep 17, 2019
…evelop

* origin/develop: (31 commits)
  Performance: Delivery optimized images (#7176)
  Add `appName` message to each locale
  Remove the disk store (#7170)
  Update @hapi/subtext as per security advisory (#7172)
  Add fixes for German translations (#7168)
  Fix recipient field of approve screen (#7171)
  3box integration 2.0 (#6972)
  ci - metamaskbot - include links to dep-viz and all artifacts (#7155)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Remove redundant error logging (#7158)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  ci - install deps with "--har" flag to capture network activity (#7143)
  ci - create source-map-explorer build-artifacts (#7141)
  ci - build-artifacts - generate sesify-viz for inspecting deps (#7151)
  Publish GitHub release from master branch (#7136)
  fix rinkeby spelling (#7148)
  deps - move gulp-terser-js to devDeps
  test:integration - fix renamed test data file
  lint fix
  ...
Gudahtt added a commit that referenced this pull request Sep 27, 2019
* origin/master:
  Add v7.2.2 to changelog
  Update minimum Firefox verison to 56.0 (#7213)
  Version v7.2.2
  Update changelog for v7.2.1, v7.2.0, and v7.1.1
  Add `appName` message to each locale
  Version v7.2.1
  Update changelog with additional bug fixes
  Fix recipient field of approve screen (#7171)
  Replace `undefined` selectedAddress with `null` (#7161)
  Add polyfill for AbortController (#7157)
  Set minimum Firefox version to v56.2 to support Waterfox (#7156)
  Publish GitHub release from master branch (#7136)
  Update the changelog for v7.1.1 (#7145)
  build - replace gulp-uglify-es with gulp-terser-js
  Version v7.2.0
  Update changelog with 7.2.0 changes
  Allow dismissing privacy mode from popup
  Add changelog
  Version v7.1.1
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.

4 participants