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.

Sign up for GitHub

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

Jump to bottom

add GIT_AUTHOR_EMAIL #6031

Merged
merged 1 commit into from
Nov 24, 2021
Merged

add GIT_AUTHOR_EMAIL #6031

merged 1 commit into from
Nov 24, 2021

Conversation

DetachHead
Copy link
Contributor

@DetachHead DetachHead commented Oct 5, 2021 •
edited
Loading

Description

GIT_AUTHOR_EMAIL also needs to be set, otherwise only the committer email is changed but the author email remains the same

Related Issue(s)

Fixes #6028

How to test

Release Notes

add `GIT_AUTHOR_EMAIL` to the environment variables mentioned in account settings

@roboquat roboquat requested a review from AlexTugarev October 5, 2021 05:09
@iQQBot
Copy link
Contributor

iQQBot commented Oct 5, 2021

And add GIT_COMMITTER_NAME ?

@DetachHead
Copy link
Contributor Author

added

@AlexTugarev AlexTugarev requested review from gtsiolis and removed request for AlexTugarev October 5, 2021 09:50
@csweichel
Copy link
Contributor

beware: this change does not actually add those environment variables. Is that intentional?

@iQQBot
Copy link
Contributor

iQQBot commented Oct 5, 2021

it just modify document

@DetachHead
Copy link
Contributor Author

In #6028 I noticed that the git commit author name only changed if this variable was set, so I assume it's already checked somewhere but just not documented, which is what my PR aims to rectify

@loujaybee
Copy link
Member

loujaybee commented Oct 6, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-main-fork.14

@akosyakov
Copy link
Member

@gtsiolis Could you have a look please?

@JanKoehnlein
Copy link
Contributor

/hold

@DetachHead have you signed a CLA?

@meysholdt
Copy link
Member

hi @DetachHead ! Thank you for your contribution! Can you please drop me an email with your name to moritz@gitpod.io? I will check if we have a CLA from you on record, and if we don't, send a CLA to you for signing via DocuSign. Sorry for the inconvenience and please understand a signed CLA is prerequisite for merging PRs in this repository.

@stale
Copy link

stale bot commented Oct 23, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Oct 23, 2021
@JanKoehnlein JanKoehnlein added meta: never-stale This issue can never become stale and removed meta: stale This issue/PR is stale and will be closed soon labels Oct 24, 2021
@loujaybee loujaybee added the team: webapp Issue belongs to the WebApp team label Nov 2, 2021
@gtsiolis gtsiolis mentioned this pull request Nov 17, 2021
Closed
@laushinka
Copy link
Contributor

@meysholdt What is the status of the CLA?

@DetachHead
Copy link
Contributor Author

sorry didn't see this, i sent you an email @meysholdt

jankeromnes
jankeromnes approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Looks good to me as is, many thanks for taking the time to fix this and find a good compromise!

Adding a temporary hold on the auto-merger until a CLA is confirmed (please unhold when it is):

/hold

@roboquat roboquat added the lgtm label Nov 19, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5e07d4fe0ae12170763592713919a80f92f66c8b

@jankeromnes
Copy link
Contributor

jankeromnes commented Nov 19, 2021 •
edited
Loading

Ah, please also fix the conflict when you get a chance (hint: it's just that all the words "Git" got capitalized since you first wrote the PR).

Also, let's run the CI to see what it looks like with the 4 variables:

/werft run

👍 started the job as gitpod-build-main-fork.15

@roboquat roboquat removed the lgtm label Nov 19, 2021
jankeromnes
jankeromnes approved these changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Still looks good to me, many thanks! 👍

Could you please also squash the 4 commits into one? 🙏

@meysholdt any news on the CLA? 👀

/lgtm
/werft run

@roboquat roboquat added the lgtm label Nov 22, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: de0187f0434bfde0d7fa453800d3e2fcba5c89ec

@DetachHead
Copy link
Contributor Author

signed the cla

@JanKoehnlein
Copy link
Contributor

@DetachHead please squash your 4 commits into one

@meysholdt
Copy link
Member

Thank you for signing the CLA!

@DetachHead
Copy link
Contributor Author

isn't there a button to do that when you merge?

@roboquat roboquat removed the lgtm label Nov 24, 2021
@jankeromnes
Copy link
Contributor

@DetachHead There is indeed a "Squash & Merge" option on the manual merge button, but we've disabled it and completely automated merges with tide (as soon as a Pull Request is approved and all tests pass), and unfortunately tide only does "Rebase & Merge". That's why we're kindly asking you to squash your commits before approving this Pull Request.

@jankeromnes
Copy link
Contributor

jankeromnes commented Nov 24, 2021 •
edited by werft-gitpod-dev-com bot
Loading

...and I just noticed that you did. Many thanks for that! 🙏 😊

/lgtm
/werft run

👍 started the job as gitpod-build-main-fork.16

@roboquat roboquat added the lgtm label Nov 24, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: de0187f0434bfde0d7fa453800d3e2fcba5c89ec

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jankeromnes

Associated issue: #6028

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jankeromnes
Copy link
Contributor

/unhold

@codecov
Copy link

codecov bot commented Nov 24, 2021 •
edited
Loading

Codecov Report

Merging #6031 (f371c24) into main (87e44ed) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6031   +/-   ##
=======================================
  Coverage   19.04%   19.04%           
=======================================
  Files           2        2           
  Lines         168      168           
=======================================
  Hits           32       32           
  Misses        134      134           
  Partials        2        2
Flag Coverage Δ
components-local-app-app-linux-amd64 19.04% <ø> (ø)
components-local-app-app-linux-arm64 ∅ <ø> (∅)
components-local-app-app-windows-386 ∅ <ø> (∅)
components-local-app-app-windows-amd64 ∅ <ø> (∅)
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87e44ed...f371c24. Read the comment docs.

@roboquat roboquat merged commit 17b1b7f into gitpod-io:main Nov 24, 2021
@jankeromnes
Copy link
Contributor

jankeromnes commented Nov 24, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Many thanks for your contribution @DetachHead! 💯 🙏

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loujaybee loujaybee loujaybee left review comments

@iQQBot iQQBot iQQBot left review comments

@jankeromnes jankeromnes jankeromnes approved these changes

@gtsiolis gtsiolis Awaiting requested review from gtsiolis

Assignees

@jankeromnes jankeromnes

Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production meta: never-stale This issue can never become stale release-note size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GIT_COMMITTER_EMAIL environment variable doesn't work
10 participants
@DetachHead @iQQBot @csweichel @loujaybee @akosyakov @JanKoehnlein @meysholdt @laushinka @roboquat @jankeromnes