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

fix(dev-infra): rebase pr script and token sanitization not working #37489

Closed
wants to merge 5 commits into from
Closed

fix(dev-infra): rebase pr script and token sanitization not working #37489

wants to merge 5 commits into from

Conversation

devversion
Copy link
Member

  • See individual commits.

@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) labels Jun 8, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 8, 2020
@devversion devversion force-pushed the fix/dev-infra-git-client-token-sanitize branch 2 times, most recently from f952b70 to a6ec3e3 Compare June 8, 2020 20:51
josephperrott
josephperrott approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

dev-infra/utils/git.ts Outdated Show resolved Hide resolved
dev-infra/utils/git.ts Show resolved Hide resolved
@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 8, 2020
@devversion
fix(dev-infra): incorrect token sanitization when no token is specified
01f1b1a 
We recently moved over the git client from the merge script to the
common dev-infra utils. This made specifying a token optional, but
it looks like the logic for sanitizing messages doesn't account
for that, and we currently add `<TOKEN>` between every message
character. e.g.

```
Executing: git <TOKEN>g<TOKEN>i<TOKEN>t<TOKEN>
<TOKEN>s<TOKEN>t<TOKEN>a<TOKEN>t<TOKEN>u<TOKEN>s<TOKEN>
```
@devversion
fix(dev-infra): rebase pr script not working
e8e6f28 
The dev-infra rebase PR script currently does not work due to
the following issues:

1. The push refspec is incorrect. It refers to the `base` of the PR, and
not to the `head` of the PR.
2. The push `--force-with-lease` option does not work in a detached head
as no remote-tracking branch is set up.
@devversion
fix(dev-infra): local changes check not working
5945883 
Looks like we broke the `hasLocalChanges` check in the git client
when we moved it over from the merge script. The problem is that
we are using `git` in the first argument of `git.run`. That
means that we under-the-hood run `git git <..>`.

This commit fixes that, but also switches to a better variant
for ensuring no local changes because it exits with non-zero
when there are local changes.
@devversion
fixup! fix(dev-infra): incorrect token sanitization when no token is …
14f32ef 
…specified

Address feedback
@devversion
fixup! fix(dev-infra): incorrect token sanitization when no token is …
975a7ce 
…specified

Fix lint
@devversion devversion force-pushed the fix/dev-infra-git-client-token-sanitize branch from 50165f7 to 975a7ce Compare June 9, 2020 13:58
atscott pushed a commit that referenced this pull request Jun 9, 2020
@devversion @atscott
fix(dev-infra): incorrect token sanitization when no token is specifi…
5d2f341 
…ed (#37489)

We recently moved over the git client from the merge script to the
common dev-infra utils. This made specifying a token optional, but
it looks like the logic for sanitizing messages doesn't account
for that, and we currently add `<TOKEN>` between every message
character. e.g.

```
Executing: git <TOKEN>g<TOKEN>i<TOKEN>t<TOKEN>
<TOKEN>s<TOKEN>t<TOKEN>a<TOKEN>t<TOKEN>u<TOKEN>s<TOKEN>
```

PR Close #37489
atscott pushed a commit that referenced this pull request Jun 9, 2020
@devversion @atscott
fix(dev-infra): rebase pr script not working (#37489)
acd69f2 
The dev-infra rebase PR script currently does not work due to
the following issues:

1. The push refspec is incorrect. It refers to the `base` of the PR, and
not to the `head` of the PR.
2. The push `--force-with-lease` option does not work in a detached head
as no remote-tracking branch is set up.

PR Close #37489
atscott pushed a commit that referenced this pull request Jun 9, 2020
@devversion @atscott
fix(dev-infra): local changes check not working (#37489)
c093390 
Looks like we broke the `hasLocalChanges` check in the git client
when we moved it over from the merge script. The problem is that
we are using `git` in the first argument of `git.run`. That
means that we under-the-hood run `git git <..>`.

This commit fixes that, but also switches to a better variant
for ensuring no local changes because it exits with non-zero
when there are local changes.

PR Close #37489
@atscott atscott closed this in bb924b6 Jun 9, 2020
atscott pushed a commit that referenced this pull request Jun 9, 2020
@devversion @atscott
fix(dev-infra): rebase pr script not working (#37489)
0eacba5 
The dev-infra rebase PR script currently does not work due to
the following issues:

1. The push refspec is incorrect. It refers to the `base` of the PR, and
not to the `head` of the PR.
2. The push `--force-with-lease` option does not work in a detached head
as no remote-tracking branch is set up.

PR Close #37489
atscott pushed a commit that referenced this pull request Jun 9, 2020
@devversion @atscott
fix(dev-infra): local changes check not working (#37489)
a331370 
Looks like we broke the `hasLocalChanges` check in the git client
when we moved it over from the merge script. The problem is that
we are using `git` in the first argument of `git.run`. That
means that we under-the-hood run `git git <..>`.

This commit fixes that, but also switches to a better variant
for ensuring no local changes because it exits with non-zero
when there are local changes.

PR Close #37489
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
@devversion @ngwattcos
fix(dev-infra): incorrect token sanitization when no token is specifi…
6a1ae3e 
…ed (angular#37489)

We recently moved over the git client from the merge script to the
common dev-infra utils. This made specifying a token optional, but
it looks like the logic for sanitizing messages doesn't account
for that, and we currently add `<TOKEN>` between every message
character. e.g.

```
Executing: git <TOKEN>g<TOKEN>i<TOKEN>t<TOKEN>
<TOKEN>s<TOKEN>t<TOKEN>a<TOKEN>t<TOKEN>u<TOKEN>s<TOKEN>
```

PR Close angular#37489
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
@devversion @ngwattcos
fix(dev-infra): rebase pr script not working (angular#37489)
273d6da 
The dev-infra rebase PR script currently does not work due to
the following issues:

1. The push refspec is incorrect. It refers to the `base` of the PR, and
not to the `head` of the PR.
2. The push `--force-with-lease` option does not work in a detached head
as no remote-tracking branch is set up.

PR Close angular#37489
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
@devversion @ngwattcos
fix(dev-infra): local changes check not working (angular#37489)
03f44cc 
Looks like we broke the `hasLocalChanges` check in the git client
when we moved it over from the merge script. The problem is that
we are using `git` in the first argument of `git.run`. That
means that we under-the-hood run `git git <..>`.

This commit fixes that, but also switches to a better variant
for ensuring no local changes because it exits with non-zero
when there are local changes.

PR Close angular#37489
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 10, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
fix(dev-infra): incorrect token sanitization when no token is specifi…
1d40aa8 
…ed (angular#37489)

We recently moved over the git client from the merge script to the
common dev-infra utils. This made specifying a token optional, but
it looks like the logic for sanitizing messages doesn't account
for that, and we currently add `<TOKEN>` between every message
character. e.g.

```
Executing: git <TOKEN>g<TOKEN>i<TOKEN>t<TOKEN>
<TOKEN>s<TOKEN>t<TOKEN>a<TOKEN>t<TOKEN>u<TOKEN>s<TOKEN>
```

PR Close angular#37489
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
fix(dev-infra): rebase pr script not working (angular#37489)
8d72b76 
The dev-infra rebase PR script currently does not work due to
the following issues:

1. The push refspec is incorrect. It refers to the `base` of the PR, and
not to the `head` of the PR.
2. The push `--force-with-lease` option does not work in a detached head
as no remote-tracking branch is set up.

PR Close angular#37489
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
@devversion @profanis
fix(dev-infra): local changes check not working (angular#37489)
1a2cb9b 
Looks like we broke the `hasLocalChanges` check in the git client
when we moved it over from the merge script. The problem is that
we are using `git` in the first argument of `git.run`. That
means that we under-the-hood run `git git <..>`.

This commit fixes that, but also switches to a better variant
for ensuring no local changes because it exits with non-zero
when there are local changes.

PR Close angular#37489
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephperrott josephperrott josephperrott approved these changes

Assignees

@josephperrott josephperrott

Labels
action: merge The PR is ready for merge by the caretaker area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@devversion @josephperrott @googlebot