Skip to content

Clarify how GITHUB_USER should be setup in the development guide.#947

Merged
istio-merge-robot merged 1 commit intoistio:masterfrom
baodongli:issue/918
Sep 27, 2017
Merged

Clarify how GITHUB_USER should be setup in the development guide.#947
istio-merge-robot merged 1 commit intoistio:masterfrom
baodongli:issue/918

Conversation

@baodongli
Copy link
Copy Markdown

@baodongli baodongli commented Sep 26, 2017

Fixes (#918)

This is a follow up change to PR #919: Clarify environment variables set in
.profile

Prior to this small change, the .profile settings were not completely obvious.
Now all profile settings that Istio should expect to use are set in one place

Release Note

NONE

Release note:

NONE

Fixes (#918)

This is a follow up change to PR #919: Clarify environment variables set in
.profile

Prior to this small change, the .profile settings were not completely obvious.
Now all profile settings that Istio should expect to use are set in one place

**Release Note**

NONE
@istio-testing
Copy link
Copy Markdown
Collaborator

Hi @baodongli. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

export GITHUB_USER=$USER # replace with actual if different
# If your github username is not the same as your local user name (saved in the
# shell variable $USER), then replace "$USER" below with your github username
export GITHUB_USER=$USER
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the original document is clear enough as it already claiming update the "$GITHUB_USER" below with your github username

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I totally agree. line 91 in the original text indicates replacing $GITHUB_USER (which = $USER) although it is possible someone might read that as "Replace "GITHUB_USER" below with your github username.

As an example, lets say the machine login is stdake and the github login is sdake, I could see people doing something like this:
export sdake=$USER

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A different way to fix this would be have the text read
replace "$USER" below with your github id

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 for this, @sdake

@gyliu513
Copy link
Copy Markdown
Member

/release-note-none

Copy link
Copy Markdown
Member

@sdake sdake left a comment

Choose a reason for hiding this comment

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

I think this could be a little confusing to some newcomers. Perhaps there is something simpler (as specified in the review) that could be used.

export GITHUB_USER=$USER # replace with actual if different
# If your github username is not the same as your local user name (saved in the
# shell variable $USER), then replace "$USER" below with your github username
export GITHUB_USER=$USER
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A different way to fix this would be have the text read
replace "$USER" below with your github id

@ldemailly
Copy link
Copy Markdown
Member

it's not dramatically better but I don't think it's worse so let's just get this in
/lgtm

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ldemailly

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

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Copy link
Copy Markdown
Member

@ldemailly ldemailly left a comment

Choose a reason for hiding this comment

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

accepting as it is now, but if the PR changes I'd like to re-review

@ldemailly
Copy link
Copy Markdown
Member

/ok-to-test

@istio-merge-robot
Copy link
Copy Markdown

/test all [submit-queue is verifying that this PR is safe to merge]

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue

@istio-merge-robot istio-merge-robot merged commit 8bb136a into istio:master Sep 27, 2017
rshriram pushed a commit that referenced this pull request Oct 30, 2017
Automatic merge from submit-queue

Clarify how GITHUB_USER should be setup in the development guide.

Fixes (#918)

This is a follow up change to PR #919: Clarify environment variables set in
.profile

Prior to this small change, the .profile settings were not completely obvious.
Now all profile settings that Istio should expect to use are set in one place

**Release Note**

NONE

**Release note**:

```release-note
NONE
```

Former-commit-id: 8bb136a
vbatts pushed a commit to vbatts/istio that referenced this pull request Oct 31, 2017
…tio#947)

Automatic merge from submit-queue

Clarify how GITHUB_USER should be setup in the development guide.

Fixes (istio#918)

This is a follow up change to PR istio#919: Clarify environment variables set in
.profile

Prior to this small change, the .profile settings were not completely obvious.
Now all profile settings that Istio should expect to use are set in one place

**Release Note**

NONE

**Release note**:

```release-note
NONE
```

Former-commit-id: 8bb136a
mandarjog pushed a commit that referenced this pull request Nov 2, 2017
Automatic merge from submit-queue

Clarify how GITHUB_USER should be setup in the development guide.

Fixes (#918)

This is a follow up change to PR #919: Clarify environment variables set in
.profile

Prior to this small change, the .profile settings were not completely obvious.
Now all profile settings that Istio should expect to use are set in one place

**Release Note**

NONE

**Release note**:

```release-note
NONE
```

Former-commit-id: 8bb136a
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
Automatic merge from submit-queue.

Bring back accidentally deleted build options in istio#736

**What this PR does / why we need it**:

istio#736 accidentally remove `-c opt` from release build and a couple of compile options, bring them back.

cc @costinm @ldemailly 

**Special notes for your reviewer**:

**Release note**:
```release-note
None
```
0x01001011 pushed a commit to thedemodrive/istio that referenced this pull request Jul 16, 2020
* minor doc fix

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* generated docs

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
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.

8 participants